Improve qemu-img check output
cover Improve qemu-img check output
qemu-img check produces messages that are hard to understand. Even worse is that in the end it just says something like "42 errors" with no further explanation. Recently I got bug reports from people who though that their image was corrupted, when in fact there were only a few leaked clusters after a crash.
This series tries to tell the user if it's real corruption, leaked clusters or something that went wrong during the check.
Kevin Wolf (2):
qemu-img check: Distinguish different kinds of errors qcow2/vdi: Change check to distinguish error cases
block.c | 9 ++- block.h | 10 ++++- block/qcow2-refcount.c | 120 ++++++++++++++++++++++++++---------------------- block/qcow2.c | 4 +- block/qcow2.h | 2 +- block/vdi.c | 10 ++-- block_int.h | 7 ++- qemu-img.c | 62 +++++++++++++++++++------ 8 files changed, 140 insertions(+), 84 deletions(-)
PATCH 1/2 qemu-img check: Distinguish different kinds of errors
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 <[email protected]>
--- block.c | 10 ++++++-- block.h | 10 ++++++++- qemu-img.c | 62 +++++++++++++++++++++++++++++++++++++++++++++-------------- 3 files changed, 63 insertions(+), 19 deletions(-)
block.c +7 -3
@@ -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 */
block.h +9 -1
@@ -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);
qemu-img.c +47 -15
86 lines not on this page · raw message
Signature · 1 line
1.6.6.1
Re: [Qemu-devel] [PATCH 1/2] qemu-img check: Distinguish different kinds of errors
At Fri, 2 Jul 2010 19:14:59 +0200,
Quoted 103 lines · Kevin Wolf
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 <[email protected]>--- block.c | 10 ++++++-- block.h | 10 ++++++++- qemu-img.c | 62 +++++++++++++++++++++++++++++++++++++++++++++-------------- 3 files changed, 63 insertions(+), 19 deletions(-)block.c +7
-3@@ -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 */ block.h +9
-1@@ -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);qemu-img.c +10
-6@@ -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);
qemu-img.c static int img_check(int argc, char **argv) · line 469
Quoted 10 lines
qemu-img.c +3
-6+ +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
qemu-img.c +1 -0
@@ -466,6 +466,7 @@ static int img_check(int argc, char **argv)if (ret == -ENOTSUP ) {error ("This image format does not support checks" ); +bdrv_delete (bs);return 1 ; }
Re: [Qemu-devel] [PATCH 1/2] qemu-img check: Distinguish different kinds of errors
Am 06.07.2010 12:51, schrieb MORITA Kazutaka:
Quoted 39 lines
At Fri, 2 Jul 2010 19:14:59 +0200,
Quoted 23 lines · Kevin Wolf
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 <[email protected]>--- block.c | 10 ++++++-- block.h | 10 ++++++++- qemu-img.c | 62 +++++++++++++++++++++++++++++++++++++++++++++-------------- 3 files changed, 63 insertions(+), 19 deletions(-)(quoted diff) +1
-6@@ -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);Review static int img_check(int argc, char **argv) · line 469
Quoted 10 lines
(quoted diff) +3
-6+ + 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.
Quoted 1 line
Kazutaka
Yes, you're right. Thanks for catching this, I'll send a new version.
Kevin
PATCH v2 1/2 qemu-img check: Distinguish different kinds of errors
In reply to Kevin WolfPeople 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 <[email protected]>
---
v2:
- Call bdrv_delete in -ENOTSUP case, too
block.c | 10 ++++++-- block.h | 10 ++++++++- qemu-img.c | 63 +++++++++++++++++++++++++++++++++++++++++++++-------------- 3 files changed, 64 insertions(+), 19 deletions(-)
block.c +7 -3
@@ -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 */
block.h +9 -1
@@ -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);
qemu-img.c +48 -15
87 lines not on this page · raw message
Signature · 1 line
1.6.6.1
PATCH 2/2 qcow2/vdi: Change check to distinguish error cases
In reply to Kevin WolfThis distinguishes between harmless leaks and real corruption. Hopefully users better understand what qemu-img check wants to tell them.
Signed-off-by Kevin Wolf <[email protected]>
--- block.c | 3 +- block/qcow2-refcount.c | 120 ++++++++++++++++++++++++++---------------------- block/qcow2.c | 4 +- block/qcow2.h | 2 +- block/vdi.c | 10 ++-- block_int.h | 7 ++- 6 files changed, 79 insertions(+), 67 deletions(-)
block.c +1 -2
@@ -721,8 +721,7 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res) }memset (res,0 ,sizeof (*res)); - res->corruptions = bs->drv->bdrv_check (bs); -return res->corruptions <0 ? res->corruptions :0 ; +return bs->drv->bdrv_check (bs, res); }/* commit COW file into the raw image */
block/qcow2-refcount.c +65 -55
315 lines not on this page · raw message
block/qcow2.c +2 -2
@@ -1146,9 +1146,9 @@ static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) } -static int qcow_check (BlockDriverState *bs) +static int qcow_check (BlockDriverState *bs,BdrvCheckResult *result) { -return qcow2_check_refcounts (bs); +return qcow2_check_refcounts (bs, result); }#if 0
block/qcow2.h +1 -1
@@ -185,7 +185,7 @@ void qcow2_create_refcount_update(QCowCreateState *s, int64_t offset,int qcow2_update_snapshot_refcount (BlockDriverState *bs,int64_t l1_table_offset,int l1_size,int addend); -int qcow2_check_refcounts (BlockDriverState *bs); +int qcow2_check_refcounts (BlockDriverState *bs,BdrvCheckResult *res);/* qcow2-cluster.c functions */ int qcow2_grow_l1_table (BlockDriverState *bs,int min_size);
block/vdi.c +5 -5
@@ -291,11 +291,10 @@ static void vdi_header_print(VdiHeader *header) }#endif -static int vdi_check (BlockDriverState *bs) +static int vdi_check (BlockDriverState *bs,BdrvCheckResult *res) {/* TODO: additional checks possible. */ BDRVVdiState *s = (BDRVVdiState *)bs->opaque; -int n_errors =0 ;uint32_t blocks_allocated =0 ;uint32_t block;uint32_t *bmap;
@@ -315,11 +314,12 @@ static int vdi_check(BlockDriverState *bs) }else {fprintf (stderr,"ERROR: block index %" PRIu32" also used by %" PRIu32"\n" , bmap[bmap_entry], bmap_entry); + res->corruptions++; } }else {fprintf (stderr,"ERROR: block index %" PRIu32" too large, is %" PRIu32"\n" , block, bmap_entry); - n_errors++; + res->corruptions++; } } }
@@ -327,12 +327,12 @@ static int vdi_check(BlockDriverState *bs)fprintf (stderr,"ERROR: allocated blocks mismatch, is %" PRIu32", should be %" PRIu32"\n" , blocks_allocated, s->header.blocks_allocated); - n_errors++; + res->corruptions++; }qemu_free (bmap); -return n_errors; +return 0 ; }static int vdi_get_info (BlockDriverState *bs,BlockDriverInfo *bdi)
block_int.h +5 -2
@@ -119,8 +119,11 @@ struct BlockDriver {QEMUOptionParameter *create_options; -/* Returns number of errors in image, -errno for internal errors */ -int (*bdrv_check)(BlockDriverState * bs); +/* +* Returns 0 for completed check, -errno for internal errors. +* The check results are stored in result. +*/ +int (*bdrv_check)(BlockDriverState * bs,BdrvCheckResult *result);void (*bdrv_debug_event)(BlockDriverState *bs,BlkDebugEvent event);
Signature · 1 line
1.6.6.1