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

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  3 +--
 blockdev-nbd.c      |  5 ++---
 nbd/server.c        | 45 ++++++++++++++++-----------------------------
 qemu-nbd.c          |  5 ++---
 4 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 65402d33964..2f9a2aeb73c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -295,6 +295,7 @@ typedef struct NBDExport NBDExport;
 typedef struct NBDClient NBDClient;

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
+                          const char *name, const char *description,
                           uint16_t nbdflags, void (*close)(NBDExport *),
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp);
@@ -306,8 +307,6 @@ void nbd_export_put(NBDExport *exp);
 BlockBackend *nbd_export_get_blockdev(NBDExport *exp);

 NBDExport *nbd_export_find(const char *name);
-void nbd_export_set_name(NBDExport *exp, const char *name);
-void nbd_export_set_description(NBDExport *exp, const char *description);
 void nbd_export_close_all(void);

 void nbd_client_new(QIOChannelSocket *sioc,
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 1d170c80b82..f5edbc27d88 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -174,14 +174,13 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
         writable = false;
     }

-    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
+    exp = nbd_export_new(bs, 0, -1, name, NULL,
+                         writable ? 0 : NBD_FLAG_READ_ONLY,
                          NULL, false, on_eject_blk, errp);
     if (!exp) {
         return;
     }

-    nbd_export_set_name(exp, name);
-
     /* The list of named exports has a strong reference to this export now and
      * our only way of accessing it is through nbd_export_find(), so we can drop
      * the strong reference that is @exp. */
diff --git a/nbd/server.c b/nbd/server.c
index 98327088cb4..676fb4886d0 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1456,6 +1456,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
 }

 NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
+                          const char *name, const char *description,
                           uint16_t nbdflags, void (*close)(NBDExport *),
                           bool writethrough, BlockBackend *on_eject_blk,
                           Error **errp)
@@ -1471,6 +1472,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
      * that BDRV_O_INACTIVE is cleared and the image is ready for write
      * access since the export could be available before migration handover.
      */
+    assert(name);
     ctx = bdrv_get_aio_context(bs);
     aio_context_acquire(ctx);
     bdrv_invalidate_cache(bs, NULL);
@@ -1494,6 +1496,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
     QTAILQ_INIT(&exp->clients);
     exp->blk = blk;
     exp->dev_offset = dev_offset;
+    exp->name = g_strdup(name);
+    exp->description = g_strdup(description);
     exp->nbdflags = nbdflags;
     exp->size = size < 0 ? blk_getlength(blk) : size;
     if (exp->size < 0) {
@@ -1513,10 +1517,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
         exp->eject_notifier.notify = nbd_eject_notifier;
         blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
     }
+    QTAILQ_INSERT_TAIL(&exports, exp, next);
+    nbd_export_get(exp);
     return exp;

 fail:
     blk_unref(blk);
+    g_free(exp->name);
+    g_free(exp->description);
     g_free(exp);
     return NULL;
 }
@@ -1533,33 +1541,6 @@ NBDExport *nbd_export_find(const char *name)
     return NULL;
 }

-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);
-    }
-    if (name != NULL) {
-        nbd_export_get(exp);
-        exp->name = g_strdup(name);
-        QTAILQ_INSERT_TAIL(&exports, exp, next);
-    }
-    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);
+    }
+    g_free(exp->description);
+    exp->description = NULL;
     nbd_export_put(exp);
 }

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 2807e132396..696bd78a2e2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1015,10 +1015,9 @@ int main(int argc, char **argv)
         }
     }

-    exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, nbd_export_closed,
+    exp = nbd_export_new(bs, dev_offset, fd_size, export_name,
+                         export_description, nbdflags, nbd_export_closed,
                          writethrough, NULL, &error_fatal);
-    nbd_export_set_name(exp, export_name);
-    nbd_export_set_description(exp, export_description);

     if (device) {
 #if HAVE_NBD_DEVICE
-- 
2.20.1

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).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   include/block/nbd.h |  3 +--
>   blockdev-nbd.c      |  5 ++---
>   nbd/server.c        | 45 ++++++++++++++++-----------------------------
>   qemu-nbd.c          |  5 ++---
>   4 files changed, 21 insertions(+), 37 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 65402d33964..2f9a2aeb73c 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -295,6 +295,7 @@ typedef struct NBDExport NBDExport;
>   typedef struct NBDClient NBDClient;
> 
>   NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
> +                          const char *name, const char *description,
>                             uint16_t nbdflags, void (*close)(NBDExport *),
>                             bool writethrough, BlockBackend *on_eject_blk,
>                             Error **errp);
> @@ -306,8 +307,6 @@ void nbd_export_put(NBDExport *exp);
>   BlockBackend *nbd_export_get_blockdev(NBDExport *exp);
> 
>   NBDExport *nbd_export_find(const char *name);
> -void nbd_export_set_name(NBDExport *exp, const char *name);
> -void nbd_export_set_description(NBDExport *exp, const char *description);
>   void nbd_export_close_all(void);
> 
>   void nbd_client_new(QIOChannelSocket *sioc,
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 1d170c80b82..f5edbc27d88 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -174,14 +174,13 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
>           writable = false;
>       }
> 
> -    exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
> +    exp = nbd_export_new(bs, 0, -1, name, NULL,
> +                         writable ? 0 : NBD_FLAG_READ_ONLY,
>                            NULL, false, on_eject_blk, errp);
>       if (!exp) {
>           return;
>       }
> 
> -    nbd_export_set_name(exp, name);
> -
>       /* The list of named exports has a strong reference to this export now and
>        * our only way of accessing it is through nbd_export_find(), so we can drop
>        * the strong reference that is @exp. */
> diff --git a/nbd/server.c b/nbd/server.c
> index 98327088cb4..676fb4886d0 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1456,6 +1456,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
>   }
> 
>   NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
> +                          const char *name, const char *description,
>                             uint16_t nbdflags, void (*close)(NBDExport *),
>                             bool writethrough, BlockBackend *on_eject_blk,
>                             Error **errp)
> @@ -1471,6 +1472,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
>        * that BDRV_O_INACTIVE is cleared and the image is ready for write
>        * access since the export could be available before migration handover.
>        */
> +    assert(name);
>       ctx = bdrv_get_aio_context(bs);
>       aio_context_acquire(ctx);
>       bdrv_invalidate_cache(bs, NULL);
> @@ -1494,6 +1496,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
>       QTAILQ_INIT(&exp->clients);
>       exp->blk = blk;
>       exp->dev_offset = dev_offset;
> +    exp->name = g_strdup(name);
> +    exp->description = g_strdup(description);
>       exp->nbdflags = nbdflags;
>       exp->size = size < 0 ? blk_getlength(blk) : size;
>       if (exp->size < 0) {
> @@ -1513,10 +1517,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
>           exp->eject_notifier.notify = nbd_eject_notifier;
>           blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier);
>       }
> +    QTAILQ_INSERT_TAIL(&exports, exp, next);
> +    nbd_export_get(exp);
>       return exp;
> 
>   fail:
>       blk_unref(blk);
> +    g_free(exp->name);
> +    g_free(exp->description);
>       g_free(exp);
>       return NULL;
>   }
> @@ -1533,33 +1541,6 @@ NBDExport *nbd_export_find(const char *name)
>       return NULL;
>   }
> 
> -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);
> -    }
> -    if (name != NULL) {
> -        nbd_export_get(exp);
> -        exp->name = g_strdup(name);
> -        QTAILQ_INSERT_TAIL(&exports, exp, next);
> -    }
> -    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>

> +    }
> +    g_free(exp->description);
> +    exp->description = NULL;
>       nbd_export_put(exp);
>   }
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 2807e132396..696bd78a2e2 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -1015,10 +1015,9 @@ int main(int argc, char **argv)
>           }
>       }
> 
> -    exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, nbd_export_closed,
> +    exp = nbd_export_new(bs, dev_offset, fd_size, export_name,
> +                         export_description, nbdflags, nbd_export_closed,
>                            writethrough, NULL, &error_fatal);
> -    nbd_export_set_name(exp, export_name);
> -    nbd_export_set_description(exp, export_description);
> 
>       if (device) {
>   #if HAVE_NBD_DEVICE
> 


-- 
Best regards,
Vladimir

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

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

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 <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir