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

[Maxim Levitsky]

On Tue, 2020-01-28 at 19:03 +0000, Dr. David Alan Gilbert wrote:
> * Maxim Levitsky (address@hidden) wrote:
> > Signed-off-by: Maxim Levitsky <address@hidden>
> 
> 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 <address@hidden>
> 
> > ---
> >  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 / address@hidden / Manchester, UK