| Message ID | 1456247799-9593-2-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23.02.2016 18:16, Kevin Wolf wrote: > This patch adds an option to the drive_add HMP command to create only a > BlockDriverState without a BlockBackend on top. > > The motivation for this is that libvirt needs to specify options to a > migration target (specifically, detect-zeroes). drive-mirror doesn't > allow specifying options, and the proper way to do this is to create the > target BDS separately with blockdev-add (where you can specify options) > and then use blockdev-mirror to that BDS. > > However, libvirt can't use blockdev-add as long as it is still > experimental, and we're expecting that it will still take some time, so > we need to resort to drive_add. > > The problem with drive_add is that so far it always created a BB, and > BDSes with a BB can't be used as a mirroring target as long as we don't > support multiple BBs per BDS - and while we're working towards that > goal, it's another thing that will still take some time. > > So to achieve the goal, the simplest solution to provide the > functionality now without adding one-off options to the mirror QMP > commands is to extend drive_add to create nodes without BBs. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > blockdev.c | 30 ++++++++++++++++++++++++++++++ > device-hotplug.c | 7 +++++++ > hmp-commands.hx | 4 ++-- > include/block/block_int.h | 2 ++ > 4 files changed, 41 insertions(+), 2 deletions(-) > Patch looks good to me (well, except for it being a pity we have to fall back on this HMP command), I only have a minor suggestion: [...] > diff --git a/hmp-commands.hx b/hmp-commands.hx > index bb52e4d..3b44e52 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -1200,8 +1200,8 @@ ETEXI > > { > .name = "drive_add", > - .args_type = "pci_addr:s,opts:s", > - .params = "[[<domain>:]<bus>:]<slot>\n" > + .args_type = "node:-n,pci_addr:s,opts:s", > + .params = "[-n] [[<domain>:]<bus>:]<slot>\n" > "[file=file][,if=type][,bus=n]\n" > "[,unit=m][,media=d][,index=i]\n" > "[,cyls=c,heads=h,secs=s[,trans=t]]\n" The description reads: > Add drive to PCI storage controller. Maybe this should be extended now? Max
Am 24.02.2016 um 18:50 hat Max Reitz geschrieben: > On 23.02.2016 18:16, Kevin Wolf wrote: > > This patch adds an option to the drive_add HMP command to create only a > > BlockDriverState without a BlockBackend on top. > > > > The motivation for this is that libvirt needs to specify options to a > > migration target (specifically, detect-zeroes). drive-mirror doesn't > > allow specifying options, and the proper way to do this is to create the > > target BDS separately with blockdev-add (where you can specify options) > > and then use blockdev-mirror to that BDS. > > > > However, libvirt can't use blockdev-add as long as it is still > > experimental, and we're expecting that it will still take some time, so > > we need to resort to drive_add. > > > > The problem with drive_add is that so far it always created a BB, and > > BDSes with a BB can't be used as a mirroring target as long as we don't > > support multiple BBs per BDS - and while we're working towards that > > goal, it's another thing that will still take some time. > > > > So to achieve the goal, the simplest solution to provide the > > functionality now without adding one-off options to the mirror QMP > > commands is to extend drive_add to create nodes without BBs. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > blockdev.c | 30 ++++++++++++++++++++++++++++++ > > device-hotplug.c | 7 +++++++ > > hmp-commands.hx | 4 ++-- > > include/block/block_int.h | 2 ++ > > 4 files changed, 41 insertions(+), 2 deletions(-) > > > > Patch looks good to me (well, except for it being a pity we have to fall > back on this HMP command), I only have a minor suggestion: > > [...] > > > diff --git a/hmp-commands.hx b/hmp-commands.hx > > index bb52e4d..3b44e52 100644 > > --- a/hmp-commands.hx > > +++ b/hmp-commands.hx > > @@ -1200,8 +1200,8 @@ ETEXI > > > > { > > .name = "drive_add", > > - .args_type = "pci_addr:s,opts:s", > > - .params = "[[<domain>:]<bus>:]<slot>\n" > > + .args_type = "node:-n,pci_addr:s,opts:s", > > + .params = "[-n] [[<domain>:]<bus>:]<slot>\n" > > "[file=file][,if=type][,bus=n]\n" > > "[,unit=m][,media=d][,index=i]\n" > > "[,cyls=c,heads=h,secs=s[,trans=t]]\n" > > The description reads: > > > Add drive to PCI storage controller. > > Maybe this should be extended now? It was already wrong before this patch, but I guess I could just add another patch to the series anyway. Kevin
On Tue 23 Feb 2016 06:16:38 PM CET, Kevin Wolf wrote: > However, libvirt can't use blockdev-add as long as it is still > experimental, and we're expecting that it will still take some time, > so we need to resort to drive_add. But how stable is the HMP API supposed to be? Berto
On 02/25/2016 06:18 AM, Alberto Garcia wrote: > On Tue 23 Feb 2016 06:16:38 PM CET, Kevin Wolf wrote: > >> However, libvirt can't use blockdev-add as long as it is still >> experimental, and we're expecting that it will still take some time, >> so we need to resort to drive_add. > > But how stable is the HMP API supposed to be? In general, it's not stable. But for this particular API, as long as we don't have a fully-working QMP replacement, we've been careful to keep HMP working unchanged.
diff --git a/blockdev.c b/blockdev.c index d4bc435..3f46bc1 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3867,6 +3867,36 @@ 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")) { + 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) { QmpOutputVisitor *ov = qmp_output_visitor_new(); diff --git a/device-hotplug.c b/device-hotplug.c index 9a7cd66..3e5cdaa 100644 --- a/device-hotplug.c +++ b/device-hotplug.c @@ -30,6 +30,7 @@ #include "qemu/config-file.h" #include "sysemu/sysemu.h" #include "monitor/monitor.h" +#include "block/block_int.h" static DriveInfo *add_init_drive(const char *optstr) { @@ -55,6 +56,12 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict) { DriveInfo *dinfo = NULL; const char *opts = qdict_get_str(qdict, "opts"); + bool node = qdict_get_try_bool(qdict, "node", false); + + if (node) { + hmp_drive_add_node(mon, opts); + return; + } dinfo = add_init_drive(opts); if (!dinfo) { diff --git a/hmp-commands.hx b/hmp-commands.hx index bb52e4d..3b44e52 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1200,8 +1200,8 @@ ETEXI { .name = "drive_add", - .args_type = "pci_addr:s,opts:s", - .params = "[[<domain>:]<bus>:]<slot>\n" + .args_type = "node:-n,pci_addr:s,opts:s", + .params = "[-n] [[<domain>:]<bus>:]<slot>\n" "[file=file][,if=type][,bus=n]\n" "[,unit=m][,media=d][,index=i]\n" "[,cyls=c,heads=h,secs=s[,trans=t]]\n" diff --git a/include/block/block_int.h b/include/block/block_int.h index 9ef823a..dda5ba0 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -694,6 +694,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, BlockCompletionFunc *cb, void *opaque, BlockJobTxn *txn, Error **errp); +void hmp_drive_add_node(Monitor *mon, const char *optstr); + void blk_set_bs(BlockBackend *blk, BlockDriverState *bs); void blk_dev_change_media_cb(BlockBackend *blk, bool load);
This patch adds an option to the drive_add HMP command to create only a BlockDriverState without a BlockBackend on top. The motivation for this is that libvirt needs to specify options to a migration target (specifically, detect-zeroes). drive-mirror doesn't allow specifying options, and the proper way to do this is to create the target BDS separately with blockdev-add (where you can specify options) and then use blockdev-mirror to that BDS. However, libvirt can't use blockdev-add as long as it is still experimental, and we're expecting that it will still take some time, so we need to resort to drive_add. The problem with drive_add is that so far it always created a BB, and BDSes with a BB can't be used as a mirroring target as long as we don't support multiple BBs per BDS - and while we're working towards that goal, it's another thing that will still take some time. So to achieve the goal, the simplest solution to provide the functionality now without adding one-off options to the mirror QMP commands is to extend drive_add to create nodes without BBs. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- blockdev.c | 30 ++++++++++++++++++++++++++++++ device-hotplug.c | 7 +++++++ hmp-commands.hx | 4 ++-- include/block/block_int.h | 2 ++ 4 files changed, 41 insertions(+), 2 deletions(-)