Re: [Qemu-devel] [PATCH v4 16/23] qemu-img: Drop redundant error message in compare

[John Snow <jsnow@redhat.com>]

On 09/13/2017 12:03 PM, Eric Blake wrote:
> If a read error is encountered during 'qemu-img compare', we
> were printing the "Error while reading offset ..." message twice.
> Update the testsuite for the improved output.
> 
> Further simplify the code by hoisting the error code conversion
> into the helper function, rather than repeating it at the callers.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: new patch
> ---
>  qemu-img.c                 | 19 +++++--------------
>  tests/qemu-iotests/074.out |  2 --
>  2 files changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index dfccebe6bc..3e1e373e8f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1196,8 +1196,10 @@ static int64_t sectors_to_bytes(int64_t sectors)
>  /*
>   * Check if passed sectors are empty (not allocated or contain only 0 bytes)
>   *
> - * Returns 0 in case sectors are filled with 0, 1 if sectors contain non-zero
> - * data and negative value on error.
> + * Intended for use by 'qemu-img compare': Returns 0 in case sectors are
> + * filled with 0, 1 if sectors contain non-zero data (this is a comparison
> + * failure), and 4 on error (the exit status for read errors), after emitting
> + * an error message.
>   *
>   * @param blk:  BlockBackend for the image
>   * @param sect_num: Number of first sector to check
> @@ -1218,7 +1220,7 @@ static int check_empty_sectors(BlockBackend *blk, int64_t sect_num,
>      if (ret < 0) {
>          error_report("Error while reading offset %" PRId64 " of %s: %s",
>                       sectors_to_bytes(sect_num), filename, strerror(-ret));
> -        return ret;
> +        return 4;
>      }
>      idx = find_nonzero(buffer, sect_count * BDRV_SECTOR_SIZE);
>      if (idx >= 0) {
> @@ -1473,11 +1475,6 @@ static int img_compare(int argc, char **argv)
>                                            filename2, buf1, quiet);
>              }
>              if (ret) {
> -                if (ret < 0) {
> -                    error_report("Error while reading offset %" PRId64 ": %s",
> -                                 sectors_to_bytes(sector_num), strerror(-ret));
> -                    ret = 4;
> -                }
>                  goto out;
>              }
>          }
> @@ -1522,12 +1519,6 @@ static int img_compare(int argc, char **argv)
>                  ret = check_empty_sectors(blk_over, sector_num, nb_sectors,
>                                            filename_over, buf1, quiet);
>                  if (ret) {
> -                    if (ret < 0) {
> -                        error_report("Error while reading offset %" PRId64
> -                                     " of %s: %s", sectors_to_bytes(sector_num),
> -                                     filename_over, strerror(-ret));
> -                        ret = 4;
> -                    }
>                      goto out;
>                  }
>              }
> diff --git a/tests/qemu-iotests/074.out b/tests/qemu-iotests/074.out
> index 8fba5aea9c..ede66c3f81 100644
> --- a/tests/qemu-iotests/074.out
> +++ b/tests/qemu-iotests/074.out
> @@ -4,7 +4,6 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
>  wrote 512/512 bytes at offset 512
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  qemu-img: Error while reading offset 0 of blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT: Input/output error
> -qemu-img: Error while reading offset 0: Input/output error
>  4
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
>  Formatting 'TEST_DIR/t.IMGFMT.2', fmt=IMGFMT size=0
> @@ -12,7 +11,6 @@ Formatting 'TEST_DIR/t.IMGFMT.2', fmt=IMGFMT size=0
>  wrote 512/512 bytes at offset 512
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  qemu-img: Error while reading offset 0 of blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT: Input/output error
> -qemu-img: Error while reading offset 0 of blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT: Input/output error
>  Warning: Image size mismatch!
>  4
>  Cleanup
> 

Hm, naively I might assume it's best for the caller to report the error
and to leave the function a nicely self-contained helper, but I won't
insist on it.

Reviewed-by: John Snow <jsnow@redhat.com>