Re: [Qemu-devel] [PATCH 1/2] vmdk: convert error reporting

[Kevin Wolf]

Am 10.10.2013 um 09:20 hat Fam Zheng geschrieben:
> Convert "fprintf(stderr,..." to error API by passing around errp to
> functions those want to report error message.
> 
> There are 2 more "fprintf(stderr,..." remaining in read/write code path.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block/vmdk.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 5d56e31..a98ad23 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -483,7 +483,7 @@ static int vmdk_init_tables(BlockDriverState *bs, 
> VmdkExtent *extent)
>  
>  static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
>                                   BlockDriverState *file,
> -                                 int flags)
> +                                 int flags, Error **errp)
>  {
>      int ret;
>      uint32_t magic;

The errp parameter is unused in this function. You should probably use
error_setg_errno() for the failure cases.

Callers generally don't distinguish different error return codes, so
converted functions could return void instead of int.

> @@ -514,11 +514,11 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
>  }
>  
>  static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
> -                               uint64_t desc_offset);
> +                               uint64_t desc_offset, Error **errp);
>  
>  static int vmdk_open_vmdk4(BlockDriverState *bs,
>                             BlockDriverState *file,
> -                           int flags)
> +                           int flags, Error **errp)
>  {
>      int ret;
>      uint32_t magic;
> @@ -534,7 +534,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>      if (header.capacity == 0) {
>          uint64_t desc_offset = le64_to_cpu(header.desc_offset);
>          if (desc_offset) {
> -            return vmdk_open_desc_file(bs, flags, desc_offset << 9);
> +            return vmdk_open_desc_file(bs, flags, desc_offset << 9, errp);
>          }
>      }
>  
> @@ -663,7 +663,7 @@ static int vmdk_parse_description(const char *desc, const 
> char *opt_name,
>  /* Open an extent file and append to bs array */
>  static int vmdk_open_sparse(BlockDriverState *bs,
>                              BlockDriverState *file,
> -                            int flags)
> +                            int flags, Error **errp)
>  {
>      uint32_t magic;
>  
> @@ -674,10 +674,10 @@ static int vmdk_open_sparse(BlockDriverState *bs,
>      magic = be32_to_cpu(magic);
>      switch (magic) {
>          case VMDK3_MAGIC:
> -            return vmdk_open_vmfs_sparse(bs, file, flags);
> +            return vmdk_open_vmfs_sparse(bs, file, flags, errp);
>              break;
>          case VMDK4_MAGIC:
> -            return vmdk_open_vmdk4(bs, file, flags);
> +            return vmdk_open_vmdk4(bs, file, flags, errp);
>              break;
>          default:
>              return -EMEDIUMTYPE;
> @@ -686,7 +686,7 @@ static int vmdk_open_sparse(BlockDriverState *bs,
>  }
>  
>  static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> -        const char *desc_file_path)
> +                              const char *desc_file_path, Error **errp)
>  {
>      int ret;
>      char access[11];
> @@ -748,13 +748,13 @@ static int vmdk_parse_extents(const char *desc, 
> BlockDriverState *bs,
>              extent->flat_start_offset = flat_offset << 9;
>          } else if (!strcmp(type, "SPARSE") || !strcmp(type, "VMFSSPARSE")) {
>              /* SPARSE extent and VMFSSPARSE extent are both "COWD" sparse 
> file*/
> -            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags);
> +            ret = vmdk_open_sparse(bs, extent_file, bs->open_flags, errp);
>              if (ret) {
>                  bdrv_unref(extent_file);
>                  return ret;
>              }
>          } else {
> -            fprintf(stderr,
> +            error_setg(errp,
>                  "VMDK: Not supported extent type \"%s\""".\n", type);
>              return -ENOTSUP;
>          }
> @@ -769,7 +769,7 @@ next_line:
>  }
>  
>  static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
> -                               uint64_t desc_offset)
> +                               uint64_t desc_offset, Error **errp)
>  {
>      int ret;
>      char *buf = NULL;
> @@ -798,13 +798,13 @@ static int vmdk_open_desc_file(BlockDriverState *bs, 
> int flags,
>          strcmp(ct, "vmfsSparse") &&
>          strcmp(ct, "twoGbMaxExtentSparse") &&
>          strcmp(ct, "twoGbMaxExtentFlat")) {
> -        fprintf(stderr,
> +        error_setg(errp,
>                  "VMDK: Not supported image type \"%s\""".\n", ct);
>          ret = -ENOTSUP;
>          goto exit;
>      }
>      s->desc_offset = 0;
> -    ret = vmdk_parse_extents(buf, bs, bs->file->filename);
> +    ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp);
>  exit:
>      g_free(buf);
>      return ret;
> @@ -816,10 +816,10 @@ static int vmdk_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      int ret;
>      BDRVVmdkState *s = bs->opaque;
>  
> -    if (vmdk_open_sparse(bs, bs->file, flags) == 0) {
> +    if (vmdk_open_sparse(bs, bs->file, flags, errp) == 0) {
>          s->desc_offset = 0x200;
>      } else {
> -        ret = vmdk_open_desc_file(bs, flags, 0);
> +        ret = vmdk_open_desc_file(bs, flags, 0, errp);
>          if (ret) {
>              goto fail;
>          }
> @@ -1517,12 +1517,12 @@ static int vmdk_create_extent(const char *filename, 
> int64_t filesize,
>  }
>  
>  static int filename_decompose(const char *filename, char *path, char *prefix,
> -        char *postfix, size_t buf_len)
> +                              char *postfix, size_t buf_len, Error **errp)
>  {
>      const char *p, *q;
>  
>      if (filename == NULL || !strlen(filename)) {
> -        fprintf(stderr, "Vmdk: no filename provided.\n");
> +        error_setg(errp, "Vmdk: no filename provided");

Here we have a  "Vmdk:" prefix and the error message starts in lower
case.

>          return VMDK_ERROR;
>      }
>      p = strrchr(filename, '/');
> @@ -1597,7 +1597,7 @@ static int vmdk_create(const char *filename, 
> QEMUOptionParameter *options,
>          "ddb.adapterType = \"%s\"\n";
>      Error *local_err = NULL;
>  
> -    if (filename_decompose(filename, path, prefix, postfix, PATH_MAX)) {
> +    if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) 
> {
>          return -EINVAL;
>      }
>      /* Read out options */
> @@ -1623,7 +1623,7 @@ static int vmdk_create(const char *filename, 
> QEMUOptionParameter *options,
>                 strcmp(adapter_type, "buslogic") &&
>                 strcmp(adapter_type, "lsilogic") &&
>                 strcmp(adapter_type, "legacyESX")) {
> -        fprintf(stderr, "VMDK: Unknown adapter type: '%s'.\n", adapter_type);
> +        error_setg(errp, "VMDK: Unknown adapter type: '%s'", adapter_type);

Here it's "VMDK:" and upper case.

>          return -EINVAL;
>      }
>      if (strcmp(adapter_type, "ide") != 0) {
> @@ -1639,7 +1639,7 @@ static int vmdk_create(const char *filename, 
> QEMUOptionParameter *options,
>                 strcmp(fmt, "twoGbMaxExtentSparse") &&
>                 strcmp(fmt, "twoGbMaxExtentFlat") &&
>                 strcmp(fmt, "streamOptimized")) {
> -        fprintf(stderr, "VMDK: Unknown subformat: %s\n", fmt);
> +        error_setg(errp, "VMDK: Unknown subformat: %s", fmt);
>          return -EINVAL;
>      }
>      split = !(strcmp(fmt, "twoGbMaxExtentFlat") &&
> @@ -1653,7 +1653,7 @@ static int vmdk_create(const char *filename, 
> QEMUOptionParameter *options,
>          desc_extent_line = "RW %lld SPARSE \"%s\"\n";
>      }
>      if (flat && backing_file) {
> -        /* not supporting backing file for flat image */
> +        error_setg(errp, "flat image can't have backing file");

No prefix at all and lower case.

Can we standardise on using no prefix and starting the error message
with a capital letter?

Kevin