Re: [Qemu-devel] [PATCH v8 06/16] nbd: Switch from close to eject notifier

2016-01-29 Thread Max Reitz 
On 28.01.2016 04:26, Fam Zheng wrote:
> On Wed, 01/27 18:59, Max Reitz wrote:
>> The NBD code uses the BDS close notifier to determine when a medium is
>> ejected. However, now it should use the BB's BDS removal notifier for
>> that instead of the BDS's close notifier.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  blockdev-nbd.c | 40 +---
>>  nbd/server.c   | 13 +
>>  2 files changed, 18 insertions(+), 35 deletions(-)
>>
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index 4a758ac..9d6a21c 100644
>> --- a/blockdev-nbd.c
>> +++ b/blockdev-nbd.c
>> @@ -45,37 +45,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error 
>> **errp)
>>  }
>>  }
>>  
>> -/*
>> - * Hook into the BlockBackend notifiers to close the export when the
>> - * backend is closed.
>> - */
>> -typedef struct NBDCloseNotifier {
>> -Notifier n;
>> -NBDExport *exp;
>> -QTAILQ_ENTRY(NBDCloseNotifier) next;
>> -} NBDCloseNotifier;
>> -
>> -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
>> -QTAILQ_HEAD_INITIALIZER(close_notifiers);
>> -
>> -static void nbd_close_notifier(Notifier *n, void *data)
>> -{
>> -NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
>> -
>> -notifier_remove(>n);
>> -QTAILQ_REMOVE(_notifiers, cn, next);
>> -
>> -nbd_export_close(cn->exp);
>> -nbd_export_put(cn->exp);
>> -g_free(cn);
>> -}
>> -
>>  void qmp_nbd_server_add(const char *device, bool has_writable, bool 
>> writable,
>>  Error **errp)
>>  {
>>  BlockBackend *blk;
>>  NBDExport *exp;
>> -NBDCloseNotifier *n;
>>  
>>  if (server_fd == -1) {
>>  error_setg(errp, "NBD server not running");
>> @@ -113,19 +87,15 @@ void qmp_nbd_server_add(const char *device, bool 
>> has_writable, bool writable,
>>  
>>  nbd_export_set_name(exp, device);
>>  
>> -n = g_new0(NBDCloseNotifier, 1);
>> -n->n.notify = nbd_close_notifier;
>> -n->exp = exp;
>> -blk_add_close_notifier(blk, >n);
>> -QTAILQ_INSERT_TAIL(_notifiers, n, next);
>> +/* 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. */
> 
> Not quite sure about the meaning of "the strong reference that is @exp", I
> guess you mean the one reference born in nbd_export_new(), which would match
> the code.  Other than this,

Yes, the reference returned by nbd_export_new(), which is then stored in
@exp. This is a strong reference, so once @exp goes out of scope, the
reference counter has to be decremented.

Max

> Reviewed-by: Fam Zheng 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v8 06/16] nbd: Switch from close to eject notifier

2016-01-27 Thread Fam Zheng 
On Wed, 01/27 18:59, Max Reitz wrote:
> The NBD code uses the BDS close notifier to determine when a medium is
> ejected. However, now it should use the BB's BDS removal notifier for
> that instead of the BDS's close notifier.
> 
> Signed-off-by: Max Reitz 
> ---
>  blockdev-nbd.c | 40 +---
>  nbd/server.c   | 13 +
>  2 files changed, 18 insertions(+), 35 deletions(-)
> 
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 4a758ac..9d6a21c 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -45,37 +45,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error 
> **errp)
>  }
>  }
>  
> -/*
> - * Hook into the BlockBackend notifiers to close the export when the
> - * backend is closed.
> - */
> -typedef struct NBDCloseNotifier {
> -Notifier n;
> -NBDExport *exp;
> -QTAILQ_ENTRY(NBDCloseNotifier) next;
> -} NBDCloseNotifier;
> -
> -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
> -QTAILQ_HEAD_INITIALIZER(close_notifiers);
> -
> -static void nbd_close_notifier(Notifier *n, void *data)
> -{
> -NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
> -
> -notifier_remove(>n);
> -QTAILQ_REMOVE(_notifiers, cn, next);
> -
> -nbd_export_close(cn->exp);
> -nbd_export_put(cn->exp);
> -g_free(cn);
> -}
> -
>  void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
>  Error **errp)
>  {
>  BlockBackend *blk;
>  NBDExport *exp;
> -NBDCloseNotifier *n;
>  
>  if (server_fd == -1) {
>  error_setg(errp, "NBD server not running");
> @@ -113,19 +87,15 @@ void qmp_nbd_server_add(const char *device, bool 
> has_writable, bool writable,
>  
>  nbd_export_set_name(exp, device);
>  
> -n = g_new0(NBDCloseNotifier, 1);
> -n->n.notify = nbd_close_notifier;
> -n->exp = exp;
> -blk_add_close_notifier(blk, >n);
> -QTAILQ_INSERT_TAIL(_notifiers, n, next);
> +/* 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. */

Not quite sure about the meaning of "the strong reference that is @exp", I
guess you mean the one reference born in nbd_export_new(), which would match
the code.  Other than this,

Reviewed-by: Fam Zheng

[Qemu-devel] [PATCH v8 06/16] nbd: Switch from close to eject notifier

2016-01-27 Thread Max Reitz 
The NBD code uses the BDS close notifier to determine when a medium is
ejected. However, now it should use the BB's BDS removal notifier for
that instead of the BDS's close notifier.

Signed-off-by: Max Reitz 
---
 blockdev-nbd.c | 40 +---
 nbd/server.c   | 13 +
 2 files changed, 18 insertions(+), 35 deletions(-)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 4a758ac..9d6a21c 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -45,37 +45,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
 }
 }
 
-/*
- * Hook into the BlockBackend notifiers to close the export when the
- * backend is closed.
- */
-typedef struct NBDCloseNotifier {
-Notifier n;
-NBDExport *exp;
-QTAILQ_ENTRY(NBDCloseNotifier) next;
-} NBDCloseNotifier;
-
-static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
-QTAILQ_HEAD_INITIALIZER(close_notifiers);
-
-static void nbd_close_notifier(Notifier *n, void *data)
-{
-NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
-
-notifier_remove(>n);
-QTAILQ_REMOVE(_notifiers, cn, next);
-
-nbd_export_close(cn->exp);
-nbd_export_put(cn->exp);
-g_free(cn);
-}
-
 void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
 Error **errp)
 {
 BlockBackend *blk;
 NBDExport *exp;
-NBDCloseNotifier *n;
 
 if (server_fd == -1) {
 error_setg(errp, "NBD server not running");
@@ -113,19 +87,15 @@ void qmp_nbd_server_add(const char *device, bool 
has_writable, bool writable,
 
 nbd_export_set_name(exp, device);
 
-n = g_new0(NBDCloseNotifier, 1);
-n->n.notify = nbd_close_notifier;
-n->exp = exp;
-blk_add_close_notifier(blk, >n);
-QTAILQ_INSERT_TAIL(_notifiers, n, next);
+/* 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. */
+nbd_export_put(exp);
 }
 
 void qmp_nbd_server_stop(Error **errp)
 {
-while (!QTAILQ_EMPTY(_notifiers)) {
-NBDCloseNotifier *cn = QTAILQ_FIRST(_notifiers);
-nbd_close_notifier(>n, nbd_export_get_blockdev(cn->exp));
-}
+nbd_export_close_all();
 
 if (server_fd != -1) {
 qemu_set_fd_handler(server_fd, NULL, NULL, NULL);
diff --git a/nbd/server.c b/nbd/server.c
index 5169b59..2045f7c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -64,6 +64,8 @@ struct NBDExport {
 QTAILQ_ENTRY(NBDExport) next;
 
 AioContext *ctx;
+
+Notifier eject_notifier;
 };
 
 static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports);
@@ -644,6 +646,12 @@ static void blk_aio_detach(void *opaque)
 exp->ctx = NULL;
 }
 
+static void nbd_eject_notifier(Notifier *n, void *data)
+{
+NBDExport *exp = container_of(n, NBDExport, eject_notifier);
+nbd_export_close(exp);
+}
+
 NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size,
   uint32_t nbdflags, void (*close)(NBDExport *),
   Error **errp)
@@ -666,6 +674,10 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t 
dev_offset, off_t size,
 exp->ctx = blk_get_aio_context(blk);
 blk_ref(blk);
 blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
+
+exp->eject_notifier.notify = nbd_eject_notifier;
+blk_add_remove_bs_notifier(blk, >eject_notifier);
+
 /*
  * NBD exports are used for non-shared storage migration.  Make sure
  * that BDRV_O_INACTIVE is cleared and the image is ready for write
@@ -745,6 +757,7 @@ void nbd_export_put(NBDExport *exp)
 }
 
 if (exp->blk) {
+notifier_remove(>eject_notifier);
 blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
 blk_aio_detach, exp);
 blk_unref(exp->blk);
-- 
2.7.0