| Message ID | 1475237406-26917-13-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 30.09.2016 14:09, Fam Zheng wrote: > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > hw/block/virtio-blk.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 3a6112f..ce65615 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -896,6 +896,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) > error_setg(errp, "num-queues property must be larger than 0"); > return; > } > + blk_lock_image(conf->conf.blk, conf->conf.lock_mode, &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > > blkconf_serial(&conf->conf, &conf->serial); > blkconf_apply_backend_options(&conf->conf); > Hmmm... Patch 3 introduced the conf.lock_mode field, but didn't do anything with it. That is a bit weird. Now you're applying it here but can't set it anywhere. That's a bit weird, too. Well, behavior won't change as this probably just means that now everything explicitly unlocked instead of implicitly "because we don't have locking yet". Maybe it would make sense to move the introduction of conf.lock_mode to an own patch and explain in the commit message that this field will be implicitly set to 0, so until the user is able to control the field, every BB (and thus BDS tree) will not be locked (thus not changing behavior). Max
On 22.10.2016 02:08, Max Reitz wrote: > On 30.09.2016 14:09, Fam Zheng wrote: >> Signed-off-by: Fam Zheng <famz@redhat.com> >> --- >> hw/block/virtio-blk.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index 3a6112f..ce65615 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -896,6 +896,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) >> error_setg(errp, "num-queues property must be larger than 0"); >> return; >> } >> + blk_lock_image(conf->conf.blk, conf->conf.lock_mode, &err); >> + if (err) { >> + error_propagate(errp, err); >> + return; >> + } >> >> blkconf_serial(&conf->conf, &conf->serial); >> blkconf_apply_backend_options(&conf->conf); >> > > Hmmm... Patch 3 introduced the conf.lock_mode field, but didn't do > anything with it. That is a bit weird. Now you're applying it here but > can't set it anywhere. That's a bit weird, too. Well, behavior won't > change as this probably just means that now everything explicitly > unlocked instead of implicitly "because we don't have locking yet". > > Maybe it would make sense to move the introduction of conf.lock_mode to > an own patch and explain in the commit message that this field will be > implicitly set to 0, so until the user is able to control the field, > every BB (and thus BDS tree) will not be locked (thus not changing > behavior). Oops, just noticed that it won't be "unlocked" but "auto-locked" (all the more reason to mention the implicit behavior somewhere). But maybe my comment for patch 15 has obsoleted this comment altogether. Max
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 3a6112f..ce65615 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -896,6 +896,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) error_setg(errp, "num-queues property must be larger than 0"); return; } + blk_lock_image(conf->conf.blk, conf->conf.lock_mode, &err); + if (err) { + error_propagate(errp, err); + return; + } blkconf_serial(&conf->conf, &conf->serial); blkconf_apply_backend_options(&conf->conf);
Signed-off-by: Fam Zheng <famz@redhat.com> --- hw/block/virtio-blk.c | 5 +++++ 1 file changed, 5 insertions(+)