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