Convert uses like

    opts = qemu_opts_create(..., &err);
    if (err) {
        ...
    }

to

    opts = qemu_opts_create(..., &err);
    if (!opts) {
        ...
    }

Eliminate error_propagate() that are now unnecessary.  Delete @err
that are now unused.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/parallels.c  |  4 ++--
 blockdev.c         |  5 ++---
 qdev-monitor.c     |  6 ++----
 util/qemu-config.c | 10 ++++------
 util/qemu-option.c | 12 ++++--------
 5 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 860dbb80a2..b15c9ac28d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -823,8 +823,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, &local_err);
-    if (local_err != NULL) {
+    opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
+    if (!opts) {
         goto fail_options;
     }
 
diff --git a/blockdev.c b/blockdev.c
index 73736a4eaf..481f36c543 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -504,9 +504,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     /* Check common options by copying from bs_opts to opts, all other options
      * stay in bs_opts for processing by bdrv_open(). */
     id = qdict_get_try_str(bs_opts, "id");
-    opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, &error);
-    if (error) {
-        error_propagate(errp, error);
+    opts = qemu_opts_create(&qemu_common_drive_opts, id, 1, errp);
+    if (!opts) {
         goto err_no_opts;
     }
 
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 40c34bb9cf..3143dd2afb 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -792,13 +792,11 @@ void hmp_info_qdm(Monitor *mon, const QDict *qdict)
 
 void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
 {
-    Error *local_err = NULL;
     QemuOpts *opts;
     DeviceState *dev;
 
-    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
+    if (!opts) {
         return;
     }
     if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 772f5a219e..c0d0e9b8ef 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -493,9 +493,8 @@ static void config_parse_qdict_section(QDict *options, QemuOptsList *opts,
         goto out;
     }
 
-    subopts = qemu_opts_create(opts, NULL, 0, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    subopts = qemu_opts_create(opts, NULL, 0, errp);
+    if (!subopts) {
         goto out;
     }
 
@@ -538,10 +537,9 @@ static void config_parse_qdict_section(QDict *options, QemuOptsList *opts,
             }
 
             opt_name = g_strdup_printf("%s.%u", opts->name, i++);
-            subopts = qemu_opts_create(opts, opt_name, 1, &local_err);
+            subopts = qemu_opts_create(opts, opt_name, 1, errp);
             g_free(opt_name);
-            if (local_err) {
-                error_propagate(errp, local_err);
+            if (!subopts) {
                 goto out;
             }
 
diff --git a/util/qemu-option.c b/util/qemu-option.c
index fd667ea523..6119f971a4 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -670,11 +670,9 @@ void qemu_opts_set(QemuOptsList *list, const char *id,
                    const char *name, const char *value, Error **errp)
 {
     QemuOpts *opts;
-    Error *local_err = NULL;
 
-    opts = qemu_opts_create(list, id, 1, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    opts = qemu_opts_create(list, id, 1, errp);
+    if (!opts) {
         return;
     }
     qemu_opt_set(opts, name, value, errp);
@@ -1011,10 +1009,8 @@ QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
     QemuOpts *opts;
     const QDictEntry *entry;
 
-    opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1,
-                            &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    opts = qemu_opts_create(list, qdict_get_try_str(qdict, "id"), 1, errp);
+    if (!opts) {
         return NULL;
     }
 
-- 
2.26.2

24.06.2020 19:43, Markus Armbruster wrote:
> Convert uses like
> 
>      opts = qemu_opts_create(..., &err);
>      if (err) {
>          ...
>      }
> 
> to
> 
>      opts = qemu_opts_create(..., &err);
>      if (!opts) {
>          ...
>      }
> 
> Eliminate error_propagate() that are now unnecessary.  Delete @err
> that are now unused.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   block/parallels.c  |  4 ++--
>   blockdev.c         |  5 ++---
>   qdev-monitor.c     |  6 ++----
>   util/qemu-config.c | 10 ++++------
>   util/qemu-option.c | 12 ++++--------
>   5 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 860dbb80a2..b15c9ac28d 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -823,8 +823,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           }
>       }
>   
> -    opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, &local_err);
> -    if (local_err != NULL) {
> +    opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
> +    if (!opts) {
>           goto fail_options;
>       }

Honestly, I don't like this hunk. as already complicated code (crossing gotos) becomes more
complicated (add one more pattern to fail_options path: no-op error_propagate).

At least, we'll need a follow-up patch, refactoring parallels_open() to drop "fail_options"
label completely.

Still, it should work and the rest is fine, so:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 24.06.2020 19:43, Markus Armbruster wrote:
>> Convert uses like
>>
>>      opts = qemu_opts_create(..., &err);
>>      if (err) {
>>          ...
>>      }
>>
>> to
>>
>>      opts = qemu_opts_create(..., &err);
>>      if (!opts) {
>>          ...
>>      }
>>
>> Eliminate error_propagate() that are now unnecessary.  Delete @err
>> that are now unused.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   block/parallels.c  |  4 ++--
>>   blockdev.c         |  5 ++---
>>   qdev-monitor.c     |  6 ++----
>>   util/qemu-config.c | 10 ++++------
>>   util/qemu-option.c | 12 ++++--------
>>   5 files changed, 14 insertions(+), 23 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 860dbb80a2..b15c9ac28d 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -823,8 +823,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>           }
>>       }
>>   -    opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0,
>> &local_err);
>> -    if (local_err != NULL) {
>> +    opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
>> +    if (!opts) {
>>           goto fail_options;
>>       }
>
> Honestly, I don't like this hunk. as already complicated code (crossing gotos) becomes more
> complicated (add one more pattern to fail_options path: no-op error_propagate).
>
> At least, we'll need a follow-up patch, refactoring parallels_open() to drop "fail_options"
> label completely.

For what it's worth, this is how it looks at the end of the series:

    static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
                              Error **errp)
    {
        BDRVParallelsState *s = bs->opaque;
        ParallelsHeader ph;
        int ret, size, i;
        QemuOpts *opts = NULL;
        Error *local_err = NULL;
        char *buf;

        bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
                                   BDRV_CHILD_IMAGE, false, errp);
        if (!bs->file) {
            return -EINVAL;
        }

        ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
        if (ret < 0) {
            goto fail;
        }

        bs->total_sectors = le64_to_cpu(ph.nb_sectors);

        if (le32_to_cpu(ph.version) != HEADER_VERSION) {
            goto fail_format;
        }
        if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
            s->off_multiplier = 1;
            bs->total_sectors = 0xffffffff & bs->total_sectors;
        } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
            s->off_multiplier = le32_to_cpu(ph.tracks);
        } else {
            goto fail_format;
        }

        s->tracks = le32_to_cpu(ph.tracks);
        if (s->tracks == 0) {
            error_setg(errp, "Invalid image: Zero sectors per track");
            ret = -EINVAL;
            goto fail;
        }
        if (s->tracks > INT32_MAX/513) {
            error_setg(errp, "Invalid image: Too big cluster");
            ret = -EFBIG;
            goto fail;
        }

        s->bat_size = le32_to_cpu(ph.bat_entries);
        if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
            error_setg(errp, "Catalog too large");
            ret = -EFBIG;
            goto fail;
        }

        size = bat_entry_off(s->bat_size);
        s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file->bs));
        s->header = qemu_try_blockalign(bs->file->bs, s->header_size);
        if (s->header == NULL) {
            ret = -ENOMEM;
            goto fail;
        }
        s->data_end = le32_to_cpu(ph.data_off);
        if (s->data_end == 0) {
            s->data_end = ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE);
        }
        if (s->data_end < s->header_size) {
            /* there is not enough unused space to fit to block align between BAT
               and actual data. We can't avoid read-modify-write... */
            s->header_size = size;
        }

        ret = bdrv_pread(bs->file, 0, s->header, s->header_size);
        if (ret < 0) {
            goto fail;
        }
        s->bat_bitmap = (uint32_t *)(s->header + 1);

        for (i = 0; i < s->bat_size; i++) {
            int64_t off = bat2sect(s, i);
            if (off >= s->data_end) {
                s->data_end = off + s->tracks;
            }
        }

        if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
            /* Image was not closed correctly. The check is mandatory */
            s->header_unclean = true;
            if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
                error_setg(errp, "parallels: Image was not closed correctly; "
                           "cannot be opened read/write");
                ret = -EACCES;
                goto fail;
            }
        }

        opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
        if (!opts) {
            goto fail_options;
        }

        if (!qemu_opts_absorb_qdict(opts, options, errp)) {
            goto fail_options;
        }

        s->prealloc_size =
            qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
        s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
        buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
        /* prealloc_mode can be downgraded later during allocate_clusters */
        s->prealloc_mode = qapi_enum_parse(&prealloc_mode_lookup, buf,
                                           PRL_PREALLOC_MODE_FALLOCATE,
                                           &local_err);
        g_free(buf);
        if (local_err != NULL) {
            error_propagate(errp, local_err);
            goto fail_options;
        }

        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_INACTIVE)) {
            s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
            ret = parallels_update_header(bs);
            if (ret < 0) {
                goto fail;
            }
        }

        s->bat_dirty_block = 4 * qemu_real_host_page_size;
        s->bat_dirty_bmap =
            bitmap_new(DIV_ROUND_UP(s->header_size, s->bat_dirty_block));

        /* Disable migration until bdrv_invalidate_cache method is added */
        error_setg(&s->migration_blocker, "The Parallels format used by node '%s' "
                   "does not support live migration",
                   bdrv_get_device_or_node_name(bs));
        ret = migrate_add_blocker(s->migration_blocker, errp);
        if (ret < 0) {
            error_free(s->migration_blocker);
            goto fail;
        }
        qemu_co_mutex_init(&s->lock);
        return 0;

    fail_format:
        error_setg(errp, "Image not in Parallels format");
    fail_options:
        ret = -EINVAL;
    fail:
        qemu_vfree(s->header);
        return ret;
    }

Care to suggest further improvements?

> Still, it should work and the rest is fine, so:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks!

01.07.2020 11:02, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 24.06.2020 19:43, Markus Armbruster wrote:
>>> Convert uses like
>>>
>>>       opts = qemu_opts_create(..., &err);
>>>       if (err) {
>>>           ...
>>>       }
>>>
>>> to
>>>
>>>       opts = qemu_opts_create(..., &err);
>>>       if (!opts) {
>>>           ...
>>>       }
>>>
>>> Eliminate error_propagate() that are now unnecessary.  Delete @err
>>> that are now unused.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    block/parallels.c  |  4 ++--
>>>    blockdev.c         |  5 ++---
>>>    qdev-monitor.c     |  6 ++----
>>>    util/qemu-config.c | 10 ++++------
>>>    util/qemu-option.c | 12 ++++--------
>>>    5 files changed, 14 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index 860dbb80a2..b15c9ac28d 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -823,8 +823,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>            }
>>>        }
>>>    -    opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0,
>>> &local_err);
>>> -    if (local_err != NULL) {
>>> +    opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
>>> +    if (!opts) {
>>>            goto fail_options;
>>>        }
>>
>> Honestly, I don't like this hunk. as already complicated code (crossing gotos) becomes more
>> complicated (add one more pattern to fail_options path: no-op error_propagate).
>>
>> At least, we'll need a follow-up patch, refactoring parallels_open() to drop "fail_options"
>> label completely.
> 
> For what it's worth, this is how it looks at the end of the series:
> 
>      static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>                                Error **errp)
>      {
>          BDRVParallelsState *s = bs->opaque;
>          ParallelsHeader ph;
>          int ret, size, i;
>          QemuOpts *opts = NULL;
>          Error *local_err = NULL;
>          char *buf;
> 
>          bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
>                                     BDRV_CHILD_IMAGE, false, errp);
>          if (!bs->file) {
>              return -EINVAL;
>          }
> 
>          ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
>          if (ret < 0) {
>              goto fail;
>          }
> 
>          bs->total_sectors = le64_to_cpu(ph.nb_sectors);
> 
>          if (le32_to_cpu(ph.version) != HEADER_VERSION) {
>              goto fail_format;
>          }
>          if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
>              s->off_multiplier = 1;
>              bs->total_sectors = 0xffffffff & bs->total_sectors;
>          } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
>              s->off_multiplier = le32_to_cpu(ph.tracks);
>          } else {
>              goto fail_format;
>          }
> 
>          s->tracks = le32_to_cpu(ph.tracks);
>          if (s->tracks == 0) {
>              error_setg(errp, "Invalid image: Zero sectors per track");
>              ret = -EINVAL;
>              goto fail;
>          }
>          if (s->tracks > INT32_MAX/513) {
>              error_setg(errp, "Invalid image: Too big cluster");
>              ret = -EFBIG;
>              goto fail;
>          }
> 
>          s->bat_size = le32_to_cpu(ph.bat_entries);
>          if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
>              error_setg(errp, "Catalog too large");
>              ret = -EFBIG;
>              goto fail;
>          }
> 
>          size = bat_entry_off(s->bat_size);
>          s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file->bs));
>          s->header = qemu_try_blockalign(bs->file->bs, s->header_size);
>          if (s->header == NULL) {
>              ret = -ENOMEM;
>              goto fail;
>          }
>          s->data_end = le32_to_cpu(ph.data_off);
>          if (s->data_end == 0) {
>              s->data_end = ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE);
>          }
>          if (s->data_end < s->header_size) {
>              /* there is not enough unused space to fit to block align between BAT
>                 and actual data. We can't avoid read-modify-write... */
>              s->header_size = size;
>          }
> 
>          ret = bdrv_pread(bs->file, 0, s->header, s->header_size);
>          if (ret < 0) {
>              goto fail;
>          }
>          s->bat_bitmap = (uint32_t *)(s->header + 1);
> 
>          for (i = 0; i < s->bat_size; i++) {
>              int64_t off = bat2sect(s, i);
>              if (off >= s->data_end) {
>                  s->data_end = off + s->tracks;
>              }
>          }
> 
>          if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>              /* Image was not closed correctly. The check is mandatory */
>              s->header_unclean = true;
>              if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
>                  error_setg(errp, "parallels: Image was not closed correctly; "
>                             "cannot be opened read/write");
>                  ret = -EACCES;
>                  goto fail;
>              }
>          }
> 
>          opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
>          if (!opts) {
>              goto fail_options;
>          }
> 
>          if (!qemu_opts_absorb_qdict(opts, options, errp)) {
>              goto fail_options;
>          }
> 
>          s->prealloc_size =
>              qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
>          s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
>          buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
>          /* prealloc_mode can be downgraded later during allocate_clusters */
>          s->prealloc_mode = qapi_enum_parse(&prealloc_mode_lookup, buf,
>                                             PRL_PREALLOC_MODE_FALLOCATE,
>                                             &local_err);
>          g_free(buf);
>          if (local_err != NULL) {
>              error_propagate(errp, local_err);
>              goto fail_options;
>          }
> 
>          if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_INACTIVE)) {
>              s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
>              ret = parallels_update_header(bs);
>              if (ret < 0) {
>                  goto fail;
>              }
>          }
> 
>          s->bat_dirty_block = 4 * qemu_real_host_page_size;
>          s->bat_dirty_bmap =
>              bitmap_new(DIV_ROUND_UP(s->header_size, s->bat_dirty_block));
> 
>          /* Disable migration until bdrv_invalidate_cache method is added */
>          error_setg(&s->migration_blocker, "The Parallels format used by node '%s' "
>                     "does not support live migration",
>                     bdrv_get_device_or_node_name(bs));
>          ret = migrate_add_blocker(s->migration_blocker, errp);
>          if (ret < 0) {
>              error_free(s->migration_blocker);
>              goto fail;
>          }
>          qemu_co_mutex_init(&s->lock);
>          return 0;
> 
>      fail_format:
>          error_setg(errp, "Image not in Parallels format");
>      fail_options:
>          ret = -EINVAL;
>      fail:
>          qemu_vfree(s->header);
>          return ret;
>      }
> 
> Care to suggest further improvements?

Looks good, no crossing gotos neither no-op error propagation, nothig to do then. Thanks!

> 
>> Still, it should work and the rest is fine, so:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> Thanks!
> 


-- 
Best regards,
Vladimir

On 6/24/20 11:43 AM, Markus Armbruster wrote:
> Convert uses like
> 
>      opts = qemu_opts_create(..., &err);
>      if (err) {
>          ...
>      }
> 
> to
> 
>      opts = qemu_opts_create(..., &err);
>      if (!opts) {
>          ...
>      }
> 
> Eliminate error_propagate() that are now unnecessary.  Delete @err
> that are now unused.
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org