[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/6] nbd: Merge nbd_export_set_name into nbd_
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/6] nbd: Merge nbd_export_set_name into nbd_export_new |
Date: |
Thu, 10 Jan 2019 08:44:38 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 |
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 <address@hidden>
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
[Qemu-devel] [PATCH v2 1/6] nbd: Only require disabled bitmap for read-only exports, Eric Blake, 2019/01/10