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 <vsement...@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
signature.asc
Description: OpenPGP digital signature
