[1/2] qemu-img check: Distinguish different kinds of errors

At Fri,  2 Jul 2010 19:14:59 +0200,
Kevin Wolf wrote:
> 
> People think that their images are corrupted when in fact there are just some
> leaked clusters. Differentiating several error cases should make the messages
> more comprehensible.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c    |   10 ++++++--
>  block.h    |   10 ++++++++-
>  qemu-img.c |   62 +++++++++++++++++++++++++++++++++++++++++++++--------------
>  3 files changed, 63 insertions(+), 19 deletions(-)
> 
> diff --git a/block.c b/block.c
> index dd6dd76..b0ceef0 100644
> --- a/block.c
> +++ b/block.c
> @@ -710,15 +710,19 @@ DeviceState *bdrv_get_attached(BlockDriverState *bs)
>  /*
>   * Run consistency checks on an image
>   *
> - * Returns the number of errors or -errno when an internal error occurs
> + * Returns 0 if the check could be completed (it doesn't mean that the image is
> + * free of errors) or -errno when an internal error occured. The results of the
> + * check are stored in res.
>   */
> -int bdrv_check(BlockDriverState *bs)
> +int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res)
>  {
>      if (bs->drv->bdrv_check == NULL) {
>          return -ENOTSUP;
>      }
>  
> -    return bs->drv->bdrv_check(bs);
> +    memset(res, 0, sizeof(*res));
> +    res->corruptions = bs->drv->bdrv_check(bs);
> +    return res->corruptions < 0 ? res->corruptions : 0;
>  }
>  
>  /* commit COW file into the raw image */
> diff --git a/block.h b/block.h
> index 3d03b3e..c2a7e4c 100644
> --- a/block.h
> +++ b/block.h
> @@ -74,7 +74,6 @@ void bdrv_close(BlockDriverState *bs);
>  int bdrv_attach(BlockDriverState *bs, DeviceState *qdev);
>  void bdrv_detach(BlockDriverState *bs, DeviceState *qdev);
>  DeviceState *bdrv_get_attached(BlockDriverState *bs);
> -int bdrv_check(BlockDriverState *bs);
>  int bdrv_read(BlockDriverState *bs, int64_t sector_num,
>                uint8_t *buf, int nb_sectors);
>  int bdrv_write(BlockDriverState *bs, int64_t sector_num,
> @@ -97,6 +96,15 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>      const char *backing_file, const char *backing_fmt);
>  void bdrv_register(BlockDriver *bdrv);
>  
> +
> +typedef struct BdrvCheckResult {
> +    int corruptions;
> +    int leaks;
> +    int check_errors;
> +} BdrvCheckResult;
> +
> +int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res);
> +
>  /* async block I/O */
>  typedef struct BlockDriverAIOCB BlockDriverAIOCB;
>  typedef void BlockDriverCompletionFunc(void *opaque, int ret);
> diff --git a/qemu-img.c b/qemu-img.c
> index 700af21..1782ac9 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -425,11 +425,20 @@ out:
>      return 0;
>  }
>  
> +/*
> + * Checks an image for consistency. Exit codes:
> + *
> + * 0 - Check completed, image is good
> + * 1 - Check not completed because of internal errors
> + * 2 - Check completed, image is corrupted
> + * 3 - Check completed, image has leaked clusters, but is good otherwise
> + */
>  static int img_check(int argc, char **argv)
>  {
>      int c, ret;
>      const char *filename, *fmt;
>      BlockDriverState *bs;
> +    BdrvCheckResult result;
>  
>      fmt = NULL;
>      for(;;) {
> @@ -453,28 +462,51 @@ static int img_check(int argc, char **argv)
>      if (!bs) {
>          return 1;
>      }
> -    ret = bdrv_check(bs);
> -    switch(ret) {
> -    case 0:
> -        printf("No errors were found on the image.\n");
> -        break;
> -    case -ENOTSUP:
> +    ret = bdrv_check(bs, &result);
> +
> +    if (ret == -ENOTSUP) {
>          error("This image format does not support checks");
> -        break;
> -    default:
> -        if (ret < 0) {
> -            error("An error occurred during the check");
> -        } else {
> -            printf("%d errors were found on the image.\n", ret);
> +        return 1;

Is it okay to call bdrv_delete(bs) before return?  It is necessary for
the sheepdog driver to pass qemu-iotests.

Kazutaka

Am 06.07.2010 12:51, schrieb MORITA Kazutaka:
> At Fri,  2 Jul 2010 19:14:59 +0200,
> Kevin Wolf wrote:
>>
>> People think that their images are corrupted when in fact there are just some
>> leaked clusters. Differentiating several error cases should make the messages
>> more comprehensible.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block.c    |   10 ++++++--
>>  block.h    |   10 ++++++++-
>>  qemu-img.c |   62 +++++++++++++++++++++++++++++++++++++++++++++--------------
>>  3 files changed, 63 insertions(+), 19 deletions(-)

>> @@ -453,28 +462,51 @@ static int img_check(int argc, char **argv)
>>      if (!bs) {
>>          return 1;
>>      }
>> -    ret = bdrv_check(bs);
>> -    switch(ret) {
>> -    case 0:
>> -        printf("No errors were found on the image.\n");
>> -        break;
>> -    case -ENOTSUP:
>> +    ret = bdrv_check(bs, &result);
>> +
>> +    if (ret == -ENOTSUP) {
>>          error("This image format does not support checks");
>> -        break;
>> -    default:
>> -        if (ret < 0) {
>> -            error("An error occurred during the check");
>> -        } else {
>> -            printf("%d errors were found on the image.\n", ret);
>> +        return 1;
> 
> Is it okay to call bdrv_delete(bs) before return?  It is necessary for
> the sheepdog driver to pass qemu-iotests.
> 
> Kazutaka

Yes, you're right. Thanks for catching this, I'll send a new version.

Kevin