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