Re: [PATCH v3 3/3] qcow2: Add errp to rebuild_refcount_structure()

[Eric Blake]

On Tue, Apr 05, 2022 at 03:46:52PM +0200, Hanna Reitz wrote:
> Instead of fprint()-ing error messages in rebuild_refcount_structure()
> and its rebuild_refcounts_write_refblocks() helper, pass them through an
> Error object to qcow2_check_refcounts() (which will then print it).
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index c5669eaa51..ed0ecfaa89 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -2465,7 +2465,8 @@ static int64_t alloc_clusters_imrt(BlockDriverState *bs,
>  static int rebuild_refcounts_write_refblocks(
>          BlockDriverState *bs, void **refcount_table, int64_t *nb_clusters,
>          int64_t first_cluster, int64_t end_cluster,
> -        uint64_t **on_disk_reftable_ptr, uint32_t 
> *on_disk_reftable_entries_ptr
> +        uint64_t **on_disk_reftable_ptr, uint32_t 
> *on_disk_reftable_entries_ptr,
> +        Error **errp
>      )
>  {
>      BDRVQcow2State *s = bs->opaque;
> @@ -2516,8 +2517,8 @@ static int rebuild_refcounts_write_refblocks(
>                                                    nb_clusters,
>                                                    &first_free_cluster);
>              if (refblock_offset < 0) {
> -                fprintf(stderr, "ERROR allocating refblock: %s\n",
> -                        strerror(-refblock_offset));
> +                error_setg_errno(errp, -refblock_offset,
> +                                 "ERROR allocating refblock");

Most uses of error_setg* don't ALL_CAPS the first word.  But this is
pre-existing, so I'm not insisting you change it here.

>                  return refblock_offset;
>              }
>  
> @@ -2539,6 +2540,7 @@ static int rebuild_refcounts_write_refblocks(
>                                    on_disk_reftable_entries *
>                                    REFTABLE_ENTRY_SIZE);
>                  if (!on_disk_reftable) {
> +                    error_setg(errp, "ERROR allocating reftable memory");
>                      return -ENOMEM;

Ah, so this is also a corner case bug fix, where we didn't have a
message on all error paths.

Reviewed-by: Eric Blake <eblake@redhat.com>

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