Patchwork [4/8] block: bdrv_img_create(): add Error ** argument

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

Comments

Luiz Capitulino - Oct. 17, 2012, 7:35 p.m.
This commit adds an Error ** argument to bdrv_img_create() and set it
appropriately on error.

Callers of bdrv_img_create() pass NULL for the new argument and still
rely on bdrv_img_create()'s return value. Next commits will change
callers to use the Error object instead.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c    | 23 +++++++++++++++++++++--
 block.h    |  2 +-
 blockdev.c |  2 +-
 qemu-img.c |  2 +-
 4 files changed, 24 insertions(+), 5 deletions(-)
Kevin Wolf - Oct. 18, 2012, 12:08 p.m.
Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> This commit adds an Error ** argument to bdrv_img_create() and set it
> appropriately on error.
> 
> Callers of bdrv_img_create() pass NULL for the new argument and still
> rely on bdrv_img_create()'s return value. Next commits will change
> callers to use the Error object instead.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  block.c    | 23 +++++++++++++++++++++--
>  block.h    |  2 +-
>  blockdev.c |  2 +-
>  qemu-img.c |  2 +-
>  4 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index bdb53af..83bcab5 100644
> --- a/block.c
> +++ b/block.c
> @@ -4295,7 +4295,7 @@ 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,
> -                    QEMUOptionParameter **param_list)
> +                    QEMUOptionParameter **param_list, Error **errp)
>  {
>      QEMUOptionParameter *param = NULL, *create_options = NULL;
>      QEMUOptionParameter *backing_fmt, *backing_file, *size;
> @@ -4308,6 +4308,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
>      drv = bdrv_find_format(fmt);
>      if (!drv) {
>          error_report("Unknown file format '%s'", fmt);
> +        error_setg(errp, "Unknown file format '%s'", fmt);
>          ret = -EINVAL;
>          goto out;
>      }
> @@ -4315,6 +4316,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
>      proto_drv = bdrv_find_protocol(filename);
>      if (!proto_drv) {
>          error_report("Unknown protocol '%s'", filename);
> +        error_setg(errp, "Unknown protocol '%s'", filename);
>          ret = -EINVAL;
>          goto out;
>      }
> @@ -4334,6 +4336,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
>          param = parse_option_parameters(options, create_options, param);
>          if (param == NULL) {
>              error_report("Invalid options for file format '%s'.", fmt);
> +            error_setg(errp, "Invalid options for file format '%s'.", fmt);
>              ret = -EINVAL;
>              goto out;
>          }
> @@ -4344,6 +4347,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
>                                   base_filename)) {
>              error_report("Backing file not supported for file format '%s'",
>                           fmt);
> +            error_setg(errp, "Backing file not supported for file format '%s'",
> +                       fmt);
>              ret = -EINVAL;
>              goto out;
>          }
> @@ -4353,6 +4358,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
>          if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>              error_report("Backing file format not supported for file "
>                           "format '%s'", fmt);
> +            error_setg(errp, "Backing file format not supported for file "
> +                             "format '%s'", fmt);
>              ret = -EINVAL;
>              goto out;
>          }
> @@ -4363,6 +4370,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
>          if (!strcmp(filename, backing_file->value.s)) {
>              error_report("Error: Trying to create an image with the "
>                           "same filename as the backing file");
> +            error_setg(errp, "Error: Trying to create an image with the "
> +                             "same filename as the backing file");
>              ret = -EINVAL;
>              goto out;
>          }
> @@ -4374,6 +4383,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
>          if (!backing_drv) {
>              error_report("Unknown backing file format '%s'",
>                           backing_fmt->value.s);
> +            error_setg(errp, "Unknown backing file format '%s'",
> +                       backing_fmt->value.s);
>              ret = -EINVAL;
>              goto out;
>          }
> @@ -4396,7 +4407,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
>  
>              ret = bdrv_open(bs, backing_file->value.s, back_flags, backing_drv);
>              if (ret < 0) {
> -                error_report("Could not open '%s'", backing_file->value.s);

This is the only error_report() that is dropped in this patch. Not sure
what is the best patch to drop them, but consistency wouldn't hurt.

If we want to keep the series bisectable, i.e. qemu-img output stays the
same for each patch, you'd have to drop them in 5/8, I think.

Kevin
Luiz Capitulino - Oct. 18, 2012, 1:41 p.m.
On Thu, 18 Oct 2012 14:08:05 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 17.10.2012 21:35, schrieb Luiz Capitulino:
> > This commit adds an Error ** argument to bdrv_img_create() and set it
> > appropriately on error.
> > 
> > Callers of bdrv_img_create() pass NULL for the new argument and still
> > rely on bdrv_img_create()'s return value. Next commits will change
> > callers to use the Error object instead.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  block.c    | 23 +++++++++++++++++++++--
> >  block.h    |  2 +-
> >  blockdev.c |  2 +-
> >  qemu-img.c |  2 +-
> >  4 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index bdb53af..83bcab5 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4295,7 +4295,7 @@ 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,
> > -                    QEMUOptionParameter **param_list)
> > +                    QEMUOptionParameter **param_list, Error **errp)
> >  {
> >      QEMUOptionParameter *param = NULL, *create_options = NULL;
> >      QEMUOptionParameter *backing_fmt, *backing_file, *size;
> > @@ -4308,6 +4308,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
> >      drv = bdrv_find_format(fmt);
> >      if (!drv) {
> >          error_report("Unknown file format '%s'", fmt);
> > +        error_setg(errp, "Unknown file format '%s'", fmt);
> >          ret = -EINVAL;
> >          goto out;
> >      }
> > @@ -4315,6 +4316,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
> >      proto_drv = bdrv_find_protocol(filename);
> >      if (!proto_drv) {
> >          error_report("Unknown protocol '%s'", filename);
> > +        error_setg(errp, "Unknown protocol '%s'", filename);
> >          ret = -EINVAL;
> >          goto out;
> >      }
> > @@ -4334,6 +4336,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
> >          param = parse_option_parameters(options, create_options, param);
> >          if (param == NULL) {
> >              error_report("Invalid options for file format '%s'.", fmt);
> > +            error_setg(errp, "Invalid options for file format '%s'.", fmt);
> >              ret = -EINVAL;
> >              goto out;
> >          }
> > @@ -4344,6 +4347,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
> >                                   base_filename)) {
> >              error_report("Backing file not supported for file format '%s'",
> >                           fmt);
> > +            error_setg(errp, "Backing file not supported for file format '%s'",
> > +                       fmt);
> >              ret = -EINVAL;
> >              goto out;
> >          }
> > @@ -4353,6 +4358,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
> >          if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> >              error_report("Backing file format not supported for file "
> >                           "format '%s'", fmt);
> > +            error_setg(errp, "Backing file format not supported for file "
> > +                             "format '%s'", fmt);
> >              ret = -EINVAL;
> >              goto out;
> >          }
> > @@ -4363,6 +4370,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
> >          if (!strcmp(filename, backing_file->value.s)) {
> >              error_report("Error: Trying to create an image with the "
> >                           "same filename as the backing file");
> > +            error_setg(errp, "Error: Trying to create an image with the "
> > +                             "same filename as the backing file");
> >              ret = -EINVAL;
> >              goto out;
> >          }
> > @@ -4374,6 +4383,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
> >          if (!backing_drv) {
> >              error_report("Unknown backing file format '%s'",
> >                           backing_fmt->value.s);
> > +            error_setg(errp, "Unknown backing file format '%s'",
> > +                       backing_fmt->value.s);
> >              ret = -EINVAL;
> >              goto out;
> >          }
> > @@ -4396,7 +4407,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
> >  
> >              ret = bdrv_open(bs, backing_file->value.s, back_flags, backing_drv);
> >              if (ret < 0) {
> > -                error_report("Could not open '%s'", backing_file->value.s);
> 
> This is the only error_report() that is dropped in this patch. Not sure
> what is the best patch to drop them, but consistency wouldn't hurt.

That's correct. I dropped it by accident because I was working on this
series and the bdrv_open() conversion at the same time and probably
mixed the two.

Will fix it for v2.

> 
> If we want to keep the series bisectable, i.e. qemu-img output stays the
> same for each patch, you'd have to drop them in 5/8, I think.
> 
> Kevin
>

Patch

diff --git a/block.c b/block.c
index bdb53af..83bcab5 100644
--- a/block.c
+++ b/block.c
@@ -4295,7 +4295,7 @@  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,
-                    QEMUOptionParameter **param_list)
+                    QEMUOptionParameter **param_list, Error **errp)
 {
     QEMUOptionParameter *param = NULL, *create_options = NULL;
     QEMUOptionParameter *backing_fmt, *backing_file, *size;
@@ -4308,6 +4308,7 @@  int bdrv_img_create(const char *filename, const char *fmt,
     drv = bdrv_find_format(fmt);
     if (!drv) {
         error_report("Unknown file format '%s'", fmt);
+        error_setg(errp, "Unknown file format '%s'", fmt);
         ret = -EINVAL;
         goto out;
     }
@@ -4315,6 +4316,7 @@  int bdrv_img_create(const char *filename, const char *fmt,
     proto_drv = bdrv_find_protocol(filename);
     if (!proto_drv) {
         error_report("Unknown protocol '%s'", filename);
+        error_setg(errp, "Unknown protocol '%s'", filename);
         ret = -EINVAL;
         goto out;
     }
@@ -4334,6 +4336,7 @@  int bdrv_img_create(const char *filename, const char *fmt,
         param = parse_option_parameters(options, create_options, param);
         if (param == NULL) {
             error_report("Invalid options for file format '%s'.", fmt);
+            error_setg(errp, "Invalid options for file format '%s'.", fmt);
             ret = -EINVAL;
             goto out;
         }
@@ -4344,6 +4347,8 @@  int bdrv_img_create(const char *filename, const char *fmt,
                                  base_filename)) {
             error_report("Backing file not supported for file format '%s'",
                          fmt);
+            error_setg(errp, "Backing file not supported for file format '%s'",
+                       fmt);
             ret = -EINVAL;
             goto out;
         }
@@ -4353,6 +4358,8 @@  int bdrv_img_create(const char *filename, const char *fmt,
         if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
             error_report("Backing file format not supported for file "
                          "format '%s'", fmt);
+            error_setg(errp, "Backing file format not supported for file "
+                             "format '%s'", fmt);
             ret = -EINVAL;
             goto out;
         }
@@ -4363,6 +4370,8 @@  int bdrv_img_create(const char *filename, const char *fmt,
         if (!strcmp(filename, backing_file->value.s)) {
             error_report("Error: Trying to create an image with the "
                          "same filename as the backing file");
+            error_setg(errp, "Error: Trying to create an image with the "
+                             "same filename as the backing file");
             ret = -EINVAL;
             goto out;
         }
@@ -4374,6 +4383,8 @@  int bdrv_img_create(const char *filename, const char *fmt,
         if (!backing_drv) {
             error_report("Unknown backing file format '%s'",
                          backing_fmt->value.s);
+            error_setg(errp, "Unknown backing file format '%s'",
+                       backing_fmt->value.s);
             ret = -EINVAL;
             goto out;
         }
@@ -4396,7 +4407,8 @@  int bdrv_img_create(const char *filename, const char *fmt,
 
             ret = bdrv_open(bs, backing_file->value.s, back_flags, backing_drv);
             if (ret < 0) {
-                error_report("Could not open '%s'", backing_file->value.s);
+                error_setg_errno(errp, -ret, "Could not open '%s'",
+                                 backing_file->value.s);
                 goto out;
             }
             bdrv_get_geometry(bs, &size);
@@ -4406,6 +4418,7 @@  int bdrv_img_create(const char *filename, const char *fmt,
             set_option_parameter(param, BLOCK_OPT_SIZE, buf);
         } else {
             error_report("Image creation needs a size parameter");
+            error_setg(errp, "Image creation needs a size parameter");
             ret = -EINVAL;
             goto out;
         }
@@ -4417,12 +4430,18 @@  int bdrv_img_create(const char *filename, const char *fmt,
         if (ret == -ENOTSUP) {
             error_report("Formatting or formatting option not supported for "
                          "file format '%s'", fmt);
+            error_setg(errp,"Formatting or formatting option not supported for "
+                            "file format '%s'", fmt);
         } else if (ret == -EFBIG) {
             error_report("The image size is too large for file format '%s'",
                          fmt);
+            error_setg(errp, "The image size is too large for file format '%s'",
+                       fmt);
         } else {
             error_report("%s: error while creating %s: %s", filename, fmt,
                          strerror(-ret));
+            error_setg(errp, "%s: error while creating %s: %s", filename, fmt,
+                       strerror(-ret));
         }
     }
 
diff --git a/block.h b/block.h
index 55d13e6..49d89a1 100644
--- a/block.h
+++ b/block.h
@@ -342,7 +342,7 @@  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,
-                    QEMUOptionParameter **param_list);
+                    QEMUOptionParameter **param_list, Error **errp);
 
 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 9dddefd..01be90f 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);
+                                  NULL, -1, flags, NULL, 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 e482443..12fb6c2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -363,7 +363,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, &params);
+                          options, img_size, BDRV_O_FLAGS, &params, NULL);
     if (ret < 0) {
         goto out;
     }