Patchwork [2/8] block: bdrv_img_create(): add param_list argument

login
register
mail settings
Submitter Luiz Capitulino
Date Oct. 17, 2012, 7:35 p.m.
Message ID <1350502556-4885-3-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/192126/
State New
Headers show

Comments

Luiz Capitulino - Oct. 17, 2012, 7:35 p.m.
If set returns a copy of the parameter list used by the block driver
to create the new image.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c    | 7 ++++++-
 block.h    | 3 ++-
 blockdev.c | 2 +-
 qemu-img.c | 2 +-
 4 files changed, 10 insertions(+), 4 deletions(-)
Kevin Wolf - Oct. 18, 2012, 11:57 a.m.
Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> If set returns a copy of the parameter list used by the block driver
> to create the new image.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  block.c    | 7 ++++++-
>  block.h    | 3 ++-
>  blockdev.c | 2 +-
>  qemu-img.c | 2 +-
>  4 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index e95f613..254a5c2 100644
> --- a/block.c
> +++ b/block.c
> @@ -4294,7 +4294,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
>  
>  int bdrv_img_create(const char *filename, const char *fmt,
>                      const char *base_filename, const char *base_fmt,
> -                    char *options, uint64_t img_size, int flags)
> +                    char *options, uint64_t img_size, int flags,
> +                    QEMUOptionParameter **param_list)
>  {
>      QEMUOptionParameter *param = NULL, *create_options = NULL;
>      QEMUOptionParameter *backing_fmt, *backing_file, *size;
> @@ -4430,6 +4431,10 @@ int bdrv_img_create(const char *filename, const char *fmt,
>      }
>  
>  out:
> +    if (param_list && ret == 0) {
> +        *param_list = append_option_parameters(NULL, param);
> +    }

If you put this above the out: label, the ret == 0 check wouldn't be
necessary.

Kevin
Luiz Capitulino - Oct. 18, 2012, 1:33 p.m.
On Thu, 18 Oct 2012 13:57:45 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> > If set returns a copy of the parameter list used by the block driver
> > to create the new image.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  block.c    | 7 ++++++-
> >  block.h    | 3 ++-
> >  blockdev.c | 2 +-
> >  qemu-img.c | 2 +-
> >  4 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index e95f613..254a5c2 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4294,7 +4294,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
> >  
> >  int bdrv_img_create(const char *filename, const char *fmt,
> >                      const char *base_filename, const char *base_fmt,
> > -                    char *options, uint64_t img_size, int flags)
> > +                    char *options, uint64_t img_size, int flags,
> > +                    QEMUOptionParameter **param_list)
> >  {
> >      QEMUOptionParameter *param = NULL, *create_options = NULL;
> >      QEMUOptionParameter *backing_fmt, *backing_file, *size;
> > @@ -4430,6 +4431,10 @@ int bdrv_img_create(const char *filename, const char *fmt,
> >      }
> >  
> >  out:
> > +    if (param_list && ret == 0) {
> > +        *param_list = append_option_parameters(NULL, param);
> > +    }
> 
> If you put this above the out: label, the ret == 0 check wouldn't be
> necessary.

I'll maintain it it there but will drop the ret check, so that we're
able to return the options on error too and thus keep the "Formatting"
message the way it's today.
Luiz Capitulino - Oct. 18, 2012, 5:18 p.m.
On Thu, 18 Oct 2012 10:33:30 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Thu, 18 Oct 2012 13:57:45 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> > > If set returns a copy of the parameter list used by the block driver
> > > to create the new image.
> > > 
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > ---
> > >  block.c    | 7 ++++++-
> > >  block.h    | 3 ++-
> > >  blockdev.c | 2 +-
> > >  qemu-img.c | 2 +-
> > >  4 files changed, 10 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index e95f613..254a5c2 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -4294,7 +4294,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
> > >  
> > >  int bdrv_img_create(const char *filename, const char *fmt,
> > >                      const char *base_filename, const char *base_fmt,
> > > -                    char *options, uint64_t img_size, int flags)
> > > +                    char *options, uint64_t img_size, int flags,
> > > +                    QEMUOptionParameter **param_list)
> > >  {
> > >      QEMUOptionParameter *param = NULL, *create_options = NULL;
> > >      QEMUOptionParameter *backing_fmt, *backing_file, *size;
> > > @@ -4430,6 +4431,10 @@ int bdrv_img_create(const char *filename, const char *fmt,
> > >      }
> > >  
> > >  out:
> > > +    if (param_list && ret == 0) {
> > > +        *param_list = append_option_parameters(NULL, param);
> > > +    }
> > 
> > If you put this above the out: label, the ret == 0 check wouldn't be
> > necessary.
> 
> I'll maintain it it there but will drop the ret check, so that we're
> able to return the options on error too and thus keep the "Formatting"
> message the way it's today.

Actually my idea didn't work. I can think of the following options:

 1. Change it to "Formatted" as you suggested. Cons: will only be
    printed after the image is created

 2. Print "Formatting 'foo', 'qcow2' " before the image is created
    and the options afterwards. Cons: weird?

 3. Don't change this. Cons: will keep spitting stuff to stdout while in
    QMP

I think we should do 2.
Kevin Wolf - Oct. 19, 2012, 9:13 a.m.
Am 18.10.2012 19:18, schrieb Luiz Capitulino:
> On Thu, 18 Oct 2012 10:33:30 -0300
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
>> On Thu, 18 Oct 2012 13:57:45 +0200
>> Kevin Wolf <kwolf@redhat.com> wrote:
>>
>>> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
>>>> If set returns a copy of the parameter list used by the block driver
>>>> to create the new image.
>>>>
>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>>> ---
>>>>  block.c    | 7 ++++++-
>>>>  block.h    | 3 ++-
>>>>  blockdev.c | 2 +-
>>>>  qemu-img.c | 2 +-
>>>>  4 files changed, 10 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index e95f613..254a5c2 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -4294,7 +4294,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
>>>>  
>>>>  int bdrv_img_create(const char *filename, const char *fmt,
>>>>                      const char *base_filename, const char *base_fmt,
>>>> -                    char *options, uint64_t img_size, int flags)
>>>> +                    char *options, uint64_t img_size, int flags,
>>>> +                    QEMUOptionParameter **param_list)
>>>>  {
>>>>      QEMUOptionParameter *param = NULL, *create_options = NULL;
>>>>      QEMUOptionParameter *backing_fmt, *backing_file, *size;
>>>> @@ -4430,6 +4431,10 @@ int bdrv_img_create(const char *filename, const char *fmt,
>>>>      }
>>>>  
>>>>  out:
>>>> +    if (param_list && ret == 0) {
>>>> +        *param_list = append_option_parameters(NULL, param);
>>>> +    }
>>>
>>> If you put this above the out: label, the ret == 0 check wouldn't be
>>> necessary.
>>
>> I'll maintain it it there but will drop the ret check, so that we're
>> able to return the options on error too and thus keep the "Formatting"
>> message the way it's today.
> 
> Actually my idea didn't work. I can think of the following options:
> 
>  1. Change it to "Formatted" as you suggested. Cons: will only be
>     printed after the image is created
> 
>  2. Print "Formatting 'foo', 'qcow2' " before the image is created
>     and the options afterwards. Cons: weird?
> 
>  3. Don't change this. Cons: will keep spitting stuff to stdout while in
>     QMP
> 
> I think we should do 2.

How about leaving the code printing the message where it is, but putting
an if (!cur_mon) around it?

Kevin
Luiz Capitulino - Oct. 19, 2012, 12:49 p.m.
On Fri, 19 Oct 2012 11:13:39 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 18.10.2012 19:18, schrieb Luiz Capitulino:
> > On Thu, 18 Oct 2012 10:33:30 -0300
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > 
> >> On Thu, 18 Oct 2012 13:57:45 +0200
> >> Kevin Wolf <kwolf@redhat.com> wrote:
> >>
> >>> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> >>>> If set returns a copy of the parameter list used by the block driver
> >>>> to create the new image.
> >>>>
> >>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >>>> ---
> >>>>  block.c    | 7 ++++++-
> >>>>  block.h    | 3 ++-
> >>>>  blockdev.c | 2 +-
> >>>>  qemu-img.c | 2 +-
> >>>>  4 files changed, 10 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/block.c b/block.c
> >>>> index e95f613..254a5c2 100644
> >>>> --- a/block.c
> >>>> +++ b/block.c
> >>>> @@ -4294,7 +4294,8 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
> >>>>  
> >>>>  int bdrv_img_create(const char *filename, const char *fmt,
> >>>>                      const char *base_filename, const char *base_fmt,
> >>>> -                    char *options, uint64_t img_size, int flags)
> >>>> +                    char *options, uint64_t img_size, int flags,
> >>>> +                    QEMUOptionParameter **param_list)
> >>>>  {
> >>>>      QEMUOptionParameter *param = NULL, *create_options = NULL;
> >>>>      QEMUOptionParameter *backing_fmt, *backing_file, *size;
> >>>> @@ -4430,6 +4431,10 @@ int bdrv_img_create(const char *filename, const char *fmt,
> >>>>      }
> >>>>  
> >>>>  out:
> >>>> +    if (param_list && ret == 0) {
> >>>> +        *param_list = append_option_parameters(NULL, param);
> >>>> +    }
> >>>
> >>> If you put this above the out: label, the ret == 0 check wouldn't be
> >>> necessary.
> >>
> >> I'll maintain it it there but will drop the ret check, so that we're
> >> able to return the options on error too and thus keep the "Formatting"
> >> message the way it's today.
> > 
> > Actually my idea didn't work. I can think of the following options:
> > 
> >  1. Change it to "Formatted" as you suggested. Cons: will only be
> >     printed after the image is created
> > 
> >  2. Print "Formatting 'foo', 'qcow2' " before the image is created
> >     and the options afterwards. Cons: weird?
> > 
> >  3. Don't change this. Cons: will keep spitting stuff to stdout while in
> >     QMP
> > 
> > I think we should do 2.
> 
> How about leaving the code printing the message where it is, but putting
> an if (!cur_mon) around it?

So ugly that I prefer to drop this patch from my series and revisit this
later :)

Patch

diff --git a/block.c b/block.c
index e95f613..254a5c2 100644
--- a/block.c
+++ b/block.c
@@ -4294,7 +4294,8 @@  bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
 
 int bdrv_img_create(const char *filename, const char *fmt,
                     const char *base_filename, const char *base_fmt,
-                    char *options, uint64_t img_size, int flags)
+                    char *options, uint64_t img_size, int flags,
+                    QEMUOptionParameter **param_list)
 {
     QEMUOptionParameter *param = NULL, *create_options = NULL;
     QEMUOptionParameter *backing_fmt, *backing_file, *size;
@@ -4430,6 +4431,10 @@  int bdrv_img_create(const char *filename, const char *fmt,
     }
 
 out:
+    if (param_list && ret == 0) {
+        *param_list = append_option_parameters(NULL, param);
+    }
+
     free_option_parameters(create_options);
     free_option_parameters(param);
 
diff --git a/block.h b/block.h
index e2d89d7..55d13e6 100644
--- a/block.h
+++ b/block.h
@@ -341,7 +341,8 @@  int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
 
 int bdrv_img_create(const char *filename, const char *fmt,
                     const char *base_filename, const char *base_fmt,
-                    char *options, uint64_t img_size, int flags);
+                    char *options, uint64_t img_size, int flags,
+                    QEMUOptionParameter **param_list);
 
 void bdrv_set_buffer_alignment(BlockDriverState *bs, int align);
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
diff --git a/blockdev.c b/blockdev.c
index 99828ad..9dddefd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -783,7 +783,7 @@  void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
             ret = bdrv_img_create(new_image_file, format,
                                   states->old_bs->filename,
                                   states->old_bs->drv->format_name,
-                                  NULL, -1, flags);
+                                  NULL, -1, flags, NULL);
             if (ret) {
                 error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
                 goto delete_and_fail;
diff --git a/qemu-img.c b/qemu-img.c
index f17f187..b841012 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -362,7 +362,7 @@  static int img_create(int argc, char **argv)
     }
 
     ret = bdrv_img_create(filename, fmt, base_filename, base_fmt,
-                          options, img_size, BDRV_O_FLAGS);
+                          options, img_size, BDRV_O_FLAGS, NULL);
 out:
     if (ret) {
         return 1;