Re: [Qemu-devel] [PATCH v2 2/6] nbd: Merge nbd_export_set_name into nbd_export_new

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3873 bytes --]

On 1/10/19 4:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> 10.01.2019 10:13, Eric Blake wrote:
>> The existing NBD code had a weird split where nbd_export_new()
>> created an export but did not add it to the list of exported
>> names until a later nbd_export_set_name() came along and grabbed
>> a second reference on the object; later, nbd_export_close()
>> drops the second reference.  But since we never change the
>> name of an NBD export while it is exposed, it is easier to just
>> inline the process of setting the name as part of creating the
>> export.
>>
>> Inline the contents of nbd_export_set_name() and
>> nbd_export_set_description() into the two points in an export
>> lifecycle where they matter, then adjust both callers to pass
>> the name up front.  Note that all callers pass a non-NULL name,
>> (passing NULL at creation was for old style servers, but we
>> removed support for that in commit 7f7dfe2a).

I need to fix that sentence:


>> -void nbd_export_set_name(NBDExport *exp, const char *name)
>> -{
>> -    if (exp->name == name) {
>> -        return;
>> -    }
>> -
>> -    nbd_export_get(exp);
>> -    if (exp->name != NULL) {
>> -        g_free(exp->name);
>> -        exp->name = NULL;
>> -        QTAILQ_REMOVE(&exports, exp, next);
>> -        nbd_export_put(exp);
>> -    }

In the old code, exp->name == NULL was possible during a second
nbd_export_close(), so this conditional needs to be preserved if
nbd_export_close() can be called more than once on the same exp.

>> -    if (name != NULL) {
>> -        nbd_export_get(exp);
>> -        exp->name = g_strdup(name);
>> -        QTAILQ_INSERT_TAIL(&exports, exp, next);
>> -    }

Whereas this conditional was only possible right after nbd_export_new(),
and was always non-NULL, hence I converted it to an assert() for a
non-NULL name and unconditional code.

>> -    nbd_export_put(exp);
>> -}
>> -
>> -void nbd_export_set_description(NBDExport *exp, const char *description)
>> -{
>> -    g_free(exp->description);
>> -    exp->description = g_strdup(description);
>> -}
>> -
>>   void nbd_export_close(NBDExport *exp)
>>   {
>>       NBDClient *client, *next;
>> @@ -1568,8 +1549,14 @@ void nbd_export_close(NBDExport *exp)
>>       QTAILQ_FOREACH_SAFE(client, &exp->clients, next, next) {
>>           client_close(client, true);
>>       }
>> -    nbd_export_set_name(exp, NULL);
>> -    nbd_export_set_description(exp, NULL);
>> +    if (exp->name) {
>> +        nbd_export_put(exp);
>> +        g_free(exp->name);
>> +        exp->name = NULL;
>> +        QTAILQ_REMOVE(&exports, exp, next);
> 
> exp->name is always non-zero, and _get and _INSERT_TAIL were done independently from name,
> so with these four lines done unconditionally:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Now, on to the analysis of whether the code in nbd_export_close() needs
a conditional - yes, it does. Making it unconditional breaks the fact
that we have nested referencing semantics: You can create an export,
then call nbd_export_get(exp) to hold a second reference to it for as
long as the export remains alive. The first time nbd_export_close() is
called, you are telling the NBD server to no longer advertise the export
(no new clients can connect, because exp->name is now NULL), but all
existing clients continue to access the export just fine. The second
time nbd_export_close() is called, exp->name is already NULL, and we are
closing out all existing clients (if any are left) - it's how we
implemented the nbd-server-remove safe vs. hard mode, and how we could
add the hide/soft modes in the future (see block.json:NbdServerRemoveMode).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]