Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/monitor/block-hmp-cmds.c | 30 ++++++++++++++++++++++++
 blockdev.c                     | 42 +++++++---------------------------
 include/block/block_int.h      |  5 ++--
 3 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index a4b1604aee..7bbe4e3814 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -33,6 +33,36 @@
 #include "monitor/hmp.h"
 #include "qemu-io.h"
 
+static void hmp_drive_add_node(Monitor *mon, const char *optstr)
+{
+    QemuOpts *opts;
+    QDict *qdict;
+    Error *local_err = NULL;
+
+    opts = qemu_opts_parse_noisily(&qemu_drive_opts, optstr, false);
+    if (!opts) {
+        return;
+    }
+
+    qdict = qemu_opts_to_qdict(opts, NULL);
+
+    if (!qdict_get_try_str(qdict, "node-name")) {
+        qobject_unref(qdict);
+        error_report("'node-name' needs to be specified");
+        goto out;
+    }
+
+    BlockDriverState *bs = bds_tree_init(qdict, &local_err);
+    if (!bs) {
+        error_report_err(local_err);
+        goto out;
+    }
+
+    bdrv_set_monitor_owned(bs);
+out:
+    qemu_opts_del(opts);
+}
+
 void hmp_drive_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
diff --git a/blockdev.c b/blockdev.c
index df43e0aaef..63805f34b5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -64,7 +64,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/throttle-options.h"
 
-static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
+QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
     QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states);
 
 static int do_open_tray(const char *blk_name, const char *qdev_id,
@@ -75,6 +75,11 @@ static void blockdev_insert_medium(bool has_device, const char *device,
                                    bool has_id, const char *id,
                                    const char *node_name, Error **errp);
 
+void bdrv_set_monitor_owned(BlockDriverState *bs)
+{
+    QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
+}
+
 static const char *const if_name[IF_COUNT] = {
     [IF_NONE] = "none",
     [IF_IDE] = "ide",
@@ -652,7 +657,7 @@ err_no_opts:
 }
 
 /* Takes the ownership of bs_opts */
-static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
+BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
 {
     int bdrv_flags = 0;
 
@@ -4201,37 +4206,6 @@ out:
     aio_context_release(aio_context);
 }
 
-void hmp_drive_add_node(Monitor *mon, const char *optstr)
-{
-    QemuOpts *opts;
-    QDict *qdict;
-    Error *local_err = NULL;
-
-    opts = qemu_opts_parse_noisily(&qemu_drive_opts, optstr, false);
-    if (!opts) {
-        return;
-    }
-
-    qdict = qemu_opts_to_qdict(opts, NULL);
-
-    if (!qdict_get_try_str(qdict, "node-name")) {
-        qobject_unref(qdict);
-        error_report("'node-name' needs to be specified");
-        goto out;
-    }
-
-    BlockDriverState *bs = bds_tree_init(qdict, &local_err);
-    if (!bs) {
-        error_report_err(local_err);
-        goto out;
-    }
-
-    QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
-
-out:
-    qemu_opts_del(opts);
-}
-
 void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 {
     BlockDriverState *bs;
@@ -4261,7 +4235,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
         goto fail;
     }
 
-    QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
+    bdrv_set_monitor_owned(bs);
 
 fail:
     visit_free(v);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dd033d0b37..10df257a61 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1217,8 +1217,6 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
                             BlockCompletionFunc *cb, void *opaque,
                             JobTxn *txn, Error **errp);
 
-void hmp_drive_add_node(Monitor *mon, const char *optstr);
-
 BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   const char *child_name,
                                   const BdrvChildRole *child_role,
@@ -1320,4 +1318,7 @@ int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
 
 int refresh_total_sectors(BlockDriverState *bs, int64_t hint);
 
+void bdrv_set_monitor_owned(BlockDriverState *bs);
+BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp);
+
 #endif /* BLOCK_INT_H */
-- 
2.17.2

* Maxim Levitsky (mlevitsk@redhat.com) wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

Looks OK to me, I'm not clear on the name for 'bdrv_set_monitor_owned'
I'd want a block person to OK that, but:


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  block/monitor/block-hmp-cmds.c | 30 ++++++++++++++++++++++++
>  blockdev.c                     | 42 +++++++---------------------------
>  include/block/block_int.h      |  5 ++--
>  3 files changed, 41 insertions(+), 36 deletions(-)
> 
> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> index a4b1604aee..7bbe4e3814 100644
> --- a/block/monitor/block-hmp-cmds.c
> +++ b/block/monitor/block-hmp-cmds.c
> @@ -33,6 +33,36 @@
>  #include "monitor/hmp.h"
>  #include "qemu-io.h"
>  
> +static void hmp_drive_add_node(Monitor *mon, const char *optstr)
> +{
> +    QemuOpts *opts;
> +    QDict *qdict;
> +    Error *local_err = NULL;
> +
> +    opts = qemu_opts_parse_noisily(&qemu_drive_opts, optstr, false);
> +    if (!opts) {
> +        return;
> +    }
> +
> +    qdict = qemu_opts_to_qdict(opts, NULL);
> +
> +    if (!qdict_get_try_str(qdict, "node-name")) {
> +        qobject_unref(qdict);
> +        error_report("'node-name' needs to be specified");
> +        goto out;
> +    }
> +
> +    BlockDriverState *bs = bds_tree_init(qdict, &local_err);
> +    if (!bs) {
> +        error_report_err(local_err);
> +        goto out;
> +    }
> +
> +    bdrv_set_monitor_owned(bs);
> +out:
> +    qemu_opts_del(opts);
> +}
> +
>  void hmp_drive_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> diff --git a/blockdev.c b/blockdev.c
> index df43e0aaef..63805f34b5 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -64,7 +64,7 @@
>  #include "qemu/main-loop.h"
>  #include "qemu/throttle-options.h"
>  
> -static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
> +QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
>      QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states);
>  
>  static int do_open_tray(const char *blk_name, const char *qdev_id,
> @@ -75,6 +75,11 @@ static void blockdev_insert_medium(bool has_device, const char *device,
>                                     bool has_id, const char *id,
>                                     const char *node_name, Error **errp);
>  
> +void bdrv_set_monitor_owned(BlockDriverState *bs)
> +{
> +    QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
> +}
> +
>  static const char *const if_name[IF_COUNT] = {
>      [IF_NONE] = "none",
>      [IF_IDE] = "ide",
> @@ -652,7 +657,7 @@ err_no_opts:
>  }
>  
>  /* Takes the ownership of bs_opts */
> -static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
> +BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
>  {
>      int bdrv_flags = 0;
>  
> @@ -4201,37 +4206,6 @@ out:
>      aio_context_release(aio_context);
>  }
>  
> -void hmp_drive_add_node(Monitor *mon, const char *optstr)
> -{
> -    QemuOpts *opts;
> -    QDict *qdict;
> -    Error *local_err = NULL;
> -
> -    opts = qemu_opts_parse_noisily(&qemu_drive_opts, optstr, false);
> -    if (!opts) {
> -        return;
> -    }
> -
> -    qdict = qemu_opts_to_qdict(opts, NULL);
> -
> -    if (!qdict_get_try_str(qdict, "node-name")) {
> -        qobject_unref(qdict);
> -        error_report("'node-name' needs to be specified");
> -        goto out;
> -    }
> -
> -    BlockDriverState *bs = bds_tree_init(qdict, &local_err);
> -    if (!bs) {
> -        error_report_err(local_err);
> -        goto out;
> -    }
> -
> -    QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
> -
> -out:
> -    qemu_opts_del(opts);
> -}
> -
>  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>  {
>      BlockDriverState *bs;
> @@ -4261,7 +4235,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>          goto fail;
>      }
>  
> -    QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
> +    bdrv_set_monitor_owned(bs);
>  
>  fail:
>      visit_free(v);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index dd033d0b37..10df257a61 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -1217,8 +1217,6 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>                              BlockCompletionFunc *cb, void *opaque,
>                              JobTxn *txn, Error **errp);
>  
> -void hmp_drive_add_node(Monitor *mon, const char *optstr);
> -
>  BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
>                                    const char *child_name,
>                                    const BdrvChildRole *child_role,
> @@ -1320,4 +1318,7 @@ int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
>  
>  int refresh_total_sectors(BlockDriverState *bs, int64_t hint);
>  
> +void bdrv_set_monitor_owned(BlockDriverState *bs);
> +BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp);
> +
>  #endif /* BLOCK_INT_H */
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On Tue, 2020-01-28 at 19:03 +0000, Dr. David Alan Gilbert wrote:
> * Maxim Levitsky (mlevitsk@redhat.com) wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> 
> Looks OK to me, I'm not clear on the name for 'bdrv_set_monitor_owned'
> I'd want a block person to OK that, but:


The name inspiration came from 'bdrv_next_monitor_owned'. To me it looks
like list of all BlockDriverState which are created by the monitor.
Also comment on 'monitor_list' link chain more or less confirms this.

Thanks for the review!

Best regards,
	Maxim Levitsky


> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> > ---
> >  block/monitor/block-hmp-cmds.c | 30 ++++++++++++++++++++++++
> >  blockdev.c                     | 42 +++++++---------------------------
> >  include/block/block_int.h      |  5 ++--
> >  3 files changed, 41 insertions(+), 36 deletions(-)
> > 
> > diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> > index a4b1604aee..7bbe4e3814 100644
> > --- a/block/monitor/block-hmp-cmds.c
> > +++ b/block/monitor/block-hmp-cmds.c
> > @@ -33,6 +33,36 @@
> >  #include "monitor/hmp.h"
> >  #include "qemu-io.h"
> >  
> > +static void hmp_drive_add_node(Monitor *mon, const char *optstr)
> > +{
> > +    QemuOpts *opts;
> > +    QDict *qdict;
> > +    Error *local_err = NULL;
> > +
> > +    opts = qemu_opts_parse_noisily(&qemu_drive_opts, optstr, false);
> > +    if (!opts) {
> > +        return;
> > +    }
> > +
> > +    qdict = qemu_opts_to_qdict(opts, NULL);
> > +
> > +    if (!qdict_get_try_str(qdict, "node-name")) {
> > +        qobject_unref(qdict);
> > +        error_report("'node-name' needs to be specified");
> > +        goto out;
> > +    }
> > +
> > +    BlockDriverState *bs = bds_tree_init(qdict, &local_err);
> > +    if (!bs) {
> > +        error_report_err(local_err);
> > +        goto out;
> > +    }
> > +
> > +    bdrv_set_monitor_owned(bs);
> > +out:
> > +    qemu_opts_del(opts);
> > +}
> > +
> >  void hmp_drive_add(Monitor *mon, const QDict *qdict)
> >  {
> >      Error *err = NULL;
> > diff --git a/blockdev.c b/blockdev.c
> > index df43e0aaef..63805f34b5 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -64,7 +64,7 @@
> >  #include "qemu/main-loop.h"
> >  #include "qemu/throttle-options.h"
> >  
> > -static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
> > +QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
> >      QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states);
> >  
> >  static int do_open_tray(const char *blk_name, const char *qdev_id,
> > @@ -75,6 +75,11 @@ static void blockdev_insert_medium(bool has_device, const char *device,
> >                                     bool has_id, const char *id,
> >                                     const char *node_name, Error **errp);
> >  
> > +void bdrv_set_monitor_owned(BlockDriverState *bs)
> > +{
> > +    QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
> > +}
> > +
> >  static const char *const if_name[IF_COUNT] = {
> >      [IF_NONE] = "none",
> >      [IF_IDE] = "ide",
> > @@ -652,7 +657,7 @@ err_no_opts:
> >  }
> >  
> >  /* Takes the ownership of bs_opts */
> > -static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
> > +BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
> >  {
> >      int bdrv_flags = 0;
> >  
> > @@ -4201,37 +4206,6 @@ out:
> >      aio_context_release(aio_context);
> >  }
> >  
> > -void hmp_drive_add_node(Monitor *mon, const char *optstr)
> > -{
> > -    QemuOpts *opts;
> > -    QDict *qdict;
> > -    Error *local_err = NULL;
> > -
> > -    opts = qemu_opts_parse_noisily(&qemu_drive_opts, optstr, false);
> > -    if (!opts) {
> > -        return;
> > -    }
> > -
> > -    qdict = qemu_opts_to_qdict(opts, NULL);
> > -
> > -    if (!qdict_get_try_str(qdict, "node-name")) {
> > -        qobject_unref(qdict);
> > -        error_report("'node-name' needs to be specified");
> > -        goto out;
> > -    }
> > -
> > -    BlockDriverState *bs = bds_tree_init(qdict, &local_err);
> > -    if (!bs) {
> > -        error_report_err(local_err);
> > -        goto out;
> > -    }
> > -
> > -    QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
> > -
> > -out:
> > -    qemu_opts_del(opts);
> > -}
> > -
> >  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
> >  {
> >      BlockDriverState *bs;
> > @@ -4261,7 +4235,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
> >          goto fail;
> >      }
> >  
> > -    QTAILQ_INSERT_TAIL(&monitor_bdrv_states, bs, monitor_list);
> > +    bdrv_set_monitor_owned(bs);
> >  
> >  fail:
> >      visit_free(v);
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index dd033d0b37..10df257a61 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -1217,8 +1217,6 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
> >                              BlockCompletionFunc *cb, void *opaque,
> >                              JobTxn *txn, Error **errp);
> >  
> > -void hmp_drive_add_node(Monitor *mon, const char *optstr);
> > -
> >  BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
> >                                    const char *child_name,
> >                                    const BdrvChildRole *child_role,
> > @@ -1320,4 +1318,7 @@ int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
> >  
> >  int refresh_total_sectors(BlockDriverState *bs, int64_t hint);
> >  
> > +void bdrv_set_monitor_owned(BlockDriverState *bs);
> > +BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp);
> > +
> >  #endif /* BLOCK_INT_H */
> > -- 
> > 2.17.2
> > 
> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK