Re: [Qemu-devel] [PATCH] nbd: Advertise multi-conn for shared read-only connections

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

17.08.2019 17:30, Eric Blake wrote:
> On 8/16/19 5:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>> +++ b/blockdev-nbd.c
>>>> @@ -189,7 +189,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>>>>        }
>>>>
>>>>        exp = nbd_export_new(bs, 0, len, name, NULL, bitmap,
>>>> -                         writable ? 0 : NBD_FLAG_READ_ONLY,
>>>> +                         writable ? 0 : NBD_FLAG_READ_ONLY, true,
>>>
>>> s/true/!writable ?
>>
>> Oh, I see, John already noticed this, it's checked in nbd_export_new anyway..
> 
> Still, since two reviewers have caught it, I'm fixing it :)

With it or without:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> 
> 
>>>> @@ -1486,6 +1486,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>>>>        perm = BLK_PERM_CONSISTENT_READ;
>>>>        if ((nbdflags & NBD_FLAG_READ_ONLY) == 0) {
>>>>            perm |= BLK_PERM_WRITE;
>>>> +    } else if (shared) {
>>>> +        nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
>>
>> For me it looks a bit strange: we already have nbdflags parameter for nbd_export_new(), why
>> to add a separate boolean to pass one of nbdflags flags?
> 
> Because I want to get rid of the nbdflags in my next patch.
> 
>>
>> Also, for qemu-nbd, shouldn't we allow -e only together with -r ?
> 
> I'm reluctant to; it might break whatever existing user is okay exposing
> it (although such users are questionable, so maybe we can argue they
> were already broken).  Maybe it's time to start a deprecation cycle?
> 


-- 
Best regards,
Vladimir