Re: [PATCH v3 11/13] monitor: Move hmp_drive_add_node to block-hmp-cmds.c

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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