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

[Vladimir Sementsov-Ogievskiy]

10.01.2019 17:44, Eric Blake wrote:
> 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

hm, looks like it works in other way
void nbd_export_close(NBDExport *exp)
{
     NBDClient *client, *next;

     nbd_export_get(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);
     nbd_export_put(exp);
}

first or second time you call it, it will close all clients.

> 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).
> 

and nbd_export_remove don't call nbd_export_close if it is safe mode and there 
are active clients.

aha, remember, we decided to not implement middle mode, which removes export 
from the list but keeps
existing connections.


But you are right that it is at least not obvious, that nbd_export_close not 
called twice.. And the fact
that it depends on (seems unrelated) name variable together with 
nbd_export_close called from
nbd_export_put and from other places (consider also my comment in 
nbd_export_put) - this all looks really
weird, and hope we'll refactor it one day.

Ok, for now it's safe to keep old condition, so exp->name != NULL is a sign 
that export exists in the list
(it worth a comment, as after removing nbd_export_set_name() it becomes more 
weird), anyway patch makes
things better, hope you'll add a comment:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

-- 
Best regards,
Vladimir