Patchwork [RFC,3/7] block: bdrv_img_create(): move param printing to qemu-img

bdrv_img_create() is being used by the transaction QMP command and
therefore shouldn't print directly to the user.

Move the param printing to qemu-img instead. Has the side effect of
only printing it when the bdrv_img_create() call succeeds, otherwise
we can print errors before the action being taken, eg:

   ~/work/virt/ ./qemu-img create -f qcow2 /foo/foo 10G
   qemu-img: /foo/foo: error while creating qcow2: No such file or directory
   Formatting '/foo/foo', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c    |  4 ----
 qemu-img.c | 10 +++++++++-
 2 files changed, 9 insertions(+), 5 deletions(-)

Il 11/10/2012 23:27, Luiz Capitulino ha scritto:
> bdrv_img_create() is being used by the transaction QMP command and
> therefore shouldn't print directly to the user.
> 
> Move the param printing to qemu-img instead. Has the side effect of
> only printing it when the bdrv_img_create() call succeeds, otherwise
> we can print errors before the action being taken, eg:
> 
>    ~/work/virt/ ./qemu-img create -f qcow2 /foo/foo 10G
>    qemu-img: /foo/foo: error while creating qcow2: No such file or directory
>    Formatting '/foo/foo', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off

It is a small regression with -monitor stdio (and also with QMP it
doesn't appear anymore in the logs).  Do we care?  What alternatives
exist besides writing a QAPI key-value store and converting the output
QEMUOptionParameters to it (which I'm not suggesting to do)?

Paolo

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  block.c    |  4 ----
>  qemu-img.c | 10 +++++++++-
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 13cf04d..235423e 100644
> --- a/block.c
> +++ b/block.c
> @@ -4411,10 +4411,6 @@ int bdrv_img_create(const char *filename, const char *fmt,
>          }
>      }
>  
> -    printf("Formatting '%s', fmt=%s ", filename, fmt);
> -    print_option_parameters(param);
> -    puts("");
> -
>      ret = bdrv_create(drv, filename, param);
>  
>      if (ret < 0) {
> diff --git a/qemu-img.c b/qemu-img.c
> index b841012..ac66459 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -301,6 +301,7 @@ static int img_create(int argc, char **argv)
>      const char *filename;
>      const char *base_filename = NULL;
>      char *options = NULL;
> +    QEMUOptionParameter *params = NULL;
>  
>      for(;;) {
>          c = getopt(argc, argv, "F:b:f:he6o:");
> @@ -362,7 +363,14 @@ static int img_create(int argc, char **argv)
>      }
>  
>      ret = bdrv_img_create(filename, fmt, base_filename, base_fmt,
> -                          options, img_size, BDRV_O_FLAGS, NULL);
> +                          options, img_size, BDRV_O_FLAGS, &params);
> +    if (ret == 0 && params) {
> +        printf("Formatting '%s', fmt=%s ", filename, fmt);
> +        print_option_parameters(params);
> +        free_option_parameters(params);
> +        puts("");
> +    }
> +
>  out:
>      if (ret) {
>          return 1;
>

On Fri, 12 Oct 2012 10:29:37 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 11/10/2012 23:27, Luiz Capitulino ha scritto:
> > bdrv_img_create() is being used by the transaction QMP command and
> > therefore shouldn't print directly to the user.
> > 
> > Move the param printing to qemu-img instead. Has the side effect of
> > only printing it when the bdrv_img_create() call succeeds, otherwise
> > we can print errors before the action being taken, eg:
> > 
> >    ~/work/virt/ ./qemu-img create -f qcow2 /foo/foo 10G
> >    qemu-img: /foo/foo: error while creating qcow2: No such file or directory
> >    Formatting '/foo/foo', fmt=qcow2 size=10737418240 encryption=off cluster_size=65536 lazy_refcounts=off
> 
> It is a small regression with -monitor stdio (and also with QMP it
> doesn't appear anymore in the logs).  Do we care? 

I don't think so. But if we do, than the current code is also wrong
as it should work with any -monitor device and not only stdio.

IMO, what's there today was really meant to be displayed when running
qemu-img.

> What alternatives
> exist besides writing a QAPI key-value store and converting the output
> QEMUOptionParameters to it (which I'm not suggesting to do)?

Yes, the right way to have this would be to add it as a return value
of the qmp command calling bdrv_img_create() (certainly not doable now
for the transaction command due to compatibility issues).

And/or add a query-block-image command and/or extend query-block to
display the image options...