Re: [Qemu-devel] [PATCH v8 12/36] virtio-blk: Apply lock-mode when realize

[Max Reitz]

On 22.10.2016 02:08, Max Reitz wrote:
> On 30.09.2016 14:09, Fam Zheng wrote:
>> Signed-off-by: Fam Zheng <address@hidden>
>> ---
>>  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

signature.asc
Description: OpenPGP digital signature