[PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in hw/block/ and include/hw/block.

2021-09-29 Thread ~farzon 
From: Farzon Lotfi 

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371

Signed-off-by: Farzon Lotfi 
---
 hw/block/fdc.c   |   4 +-
 hw/block/nand.c  | 212 +++
 hw/block/onenand.c   | 126 +++
 hw/block/tc58128.c   | 136 -
 include/hw/block/flash.h |  20 ++--
 5 files changed, 249 insertions(+), 249 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 9014cd30b3..347343af35 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -595,8 +595,8 @@ enum {
 };
 
 enum {
-FD_STATE_MULTI  = 0x01,/* multi track flag */
-FD_STATE_FORMAT = 0x02,/* format flag */
+FD_STATE_MULTI  = 0x01, /* multi track flag */
+FD_STATE_FORMAT = 0x02, /* format flag */
 };
 
 enum {
diff --git a/hw/block/nand.c b/hw/block/nand.c
index 8bc80e3514..781a27d2e1 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -30,33 +30,33 @@
 #include "qemu/module.h"
 #include "qom/object.h"
 
-# define NAND_CMD_READ00x00
-# define NAND_CMD_READ10x01
-# define NAND_CMD_READ20x50
-# define NAND_CMD_LPREAD2  0x30
-# define NAND_CMD_NOSERIALREAD20x35
-# define NAND_CMD_RANDOMREAD1  0x05
-# define NAND_CMD_RANDOMREAD2  0xe0
-# define NAND_CMD_READID   0x90
-# define NAND_CMD_RESET0xff
-# define NAND_CMD_PAGEPROGRAM1 0x80
-# define NAND_CMD_PAGEPROGRAM2 0x10
-# define NAND_CMD_CACHEPROGRAM20x15
-# define NAND_CMD_BLOCKERASE1  0x60
-# define NAND_CMD_BLOCKERASE2  0xd0
-# define NAND_CMD_READSTATUS   0x70
-# define NAND_CMD_COPYBACKPRG1 0x85
-
-# define NAND_IOSTATUS_ERROR   (1 << 0)
-# define NAND_IOSTATUS_PLANE0  (1 << 1)
-# define NAND_IOSTATUS_PLANE1  (1 << 2)
-# define NAND_IOSTATUS_PLANE2  (1 << 3)
-# define NAND_IOSTATUS_PLANE3  (1 << 4)
+# define NAND_CMD_READ0 0x00
+# define NAND_CMD_READ1 0x01
+# define NAND_CMD_READ2 0x50
+# define NAND_CMD_LPREAD2   0x30
+# define NAND_CMD_NOSERIALREAD2 0x35
+# define NAND_CMD_RANDOMREAD1   0x05
+# define NAND_CMD_RANDOMREAD2   0xe0
+# define NAND_CMD_READID0x90
+# define NAND_CMD_RESET 0xff
+# define NAND_CMD_PAGEPROGRAM1  0x80
+# define NAND_CMD_PAGEPROGRAM2  0x10
+# define NAND_CMD_CACHEPROGRAM2 0x15
+# define NAND_CMD_BLOCKERASE1   0x60
+# define NAND_CMD_BLOCKERASE2   0xd0
+# define NAND_CMD_READSTATUS0x70
+# define NAND_CMD_COPYBACKPRG1  0x85
+
+# define NAND_IOSTATUS_ERROR(1 << 0)
+# define NAND_IOSTATUS_PLANE0   (1 << 1)
+# define NAND_IOSTATUS_PLANE1   (1 << 2)
+# define NAND_IOSTATUS_PLANE2   (1 << 3)
+# define NAND_IOSTATUS_PLANE3   (1 << 4)
 # define NAND_IOSTATUS_READY(1 << 6)
-# define NAND_IOSTATUS_UNPROTCT(1 << 7)
+# define NAND_IOSTATUS_UNPROTCT (1 << 7)
 
-# define MAX_PAGE  0x800
-# define MAX_OOB   0x40
+# define MAX_PAGE   0x800
+# define MAX_OOB0x40
 
 typedef struct NANDFlashState NANDFlashState;
 struct NANDFlashState {
@@ -102,40 +102,40 @@ static void mem_and(uint8_t *dest, const uint8_t *src, 
size_t n)
 }
 }
 
-# define NAND_NO_AUTOINCR  0x0001
-# define NAND_BUSWIDTH_16  0x0002
-# define NAND_NO_PADDING   0x0004
-# define NAND_CACHEPRG 0x0008
-# define NAND_COPYBACK 0x0010
-# define NAND_IS_AND   0x0020
-# define NAND_4PAGE_ARRAY  0x0040
-# define NAND_NO_READRDY   0x0100
-# define NAND_SAMSUNG_LP   (NAND_NO_PADDING | NAND_COPYBACK)
+# define NAND_NO_AUTOINCR   0x0001
+# define NAND_BUSWIDTH_16   0x0002
+# define NAND_NO_PADDING0x0004
+# define NAND_CACHEPRG  0x0008
+# define NAND_COPYBACK  0x0010
+# define NAND_IS_AND0x0020
+# define NAND_4PAGE_ARRAY   0x0040
+# define NAND_NO_READRDY0x0100
+# define NAND_SAMSUNG_LP(NAND_NO_PADDING | NAND_COPYBACK)
 
 # define NAND_IO
 
-# define PAGE(addr)((addr) >> ADDR_SHIFT)
+# define PAGE(addr) ((addr) >> ADDR_SHIFT)
 # define PAGE_START(page)   (PAGE(page) * (NAND_PAGE_SIZE + OOB_SIZE))
-# define PAGE_MASK ((1 << ADDR_SHIFT) - 1)
-# define OOB_SHIFT (PAGE_SHIFT - 5)
-# define OOB_SIZE  (1 << OOB_SHIFT)
-# define SECTOR(addr)  ((addr) >> (9 + ADDR_SHIFT - PAGE_SHIFT))
-# define SECTOR_OFFSET(addr)   ((addr) & ((511 >> PAGE_SHIFT) << 8))
+# define PAGE_MASK  ((1 << ADDR_SHIFT) - 1)
+# define OOB_SHIFT  (PAGE_SHIFT - 5)
+# define OOB_SIZE   (1 << OOB_SHIFT)
+# define SECTOR(addr)   ((addr) >> (9 + ADDR_SHIFT - PAGE_SHIFT))
+# define SECTOR_OFFSET(addr)((addr) & ((511 >> PAGE_SHIFT) << 8))
 
 # define NAND_PAGE_SIZE 256
-# define PAGE_SHIFT8
-# define PAGE_SECTORS  1
-# define ADDR_SHIFT8
+# define PAGE_SHIFT 8
+# define PAGE_SECTORS   1
+# define ADDR_SHIFT 8
 # include "nand.c"
 # define NAND_PAGE_SIZE 512
-# define PAGE_SHIFT9
-# define

[PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in hw/ide/ and include/hw/ide.

2021-09-29 Thread ~farzon 
From: Farzon Lotfi 

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371

Signed-off-by: Farzon Lotfi 
---
 hw/ide/cmd646.c   |  28 +--
 hw/ide/core.c |  84 -
 hw/ide/microdrive.c   | 360 +++---
 include/hw/ide/internal.h | 248 +-
 4 files changed, 360 insertions(+), 360 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index c254631485..d58f1cce45 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -36,20 +36,20 @@
 #include "trace.h"
 
 /* CMD646 specific */
-#define CFR0x50
-#define   CFR_INTR_CH0 0x04
-#define CNTRL  0x51
-#define   CNTRL_EN_CH0 0x04
-#define   CNTRL_EN_CH1 0x08
-#define ARTTIM23   0x57
-#defineARTTIM23_INTR_CH1   0x10
-#define MRDMODE0x71
-#define   MRDMODE_INTR_CH0 0x04
-#define   MRDMODE_INTR_CH1 0x08
-#define   MRDMODE_BLK_CH0  0x10
-#define   MRDMODE_BLK_CH1  0x20
-#define UDIDETCR0  0x73
-#define UDIDETCR1  0x7B
+#define CFR 0x50
+#define   CFR_INTR_CH0  0x04
+#define CNTRL   0x51
+#define   CNTRL_EN_CH0  0x04
+#define   CNTRL_EN_CH1  0x08
+#define ARTTIM230x57
+#defineARTTIM23_INTR_CH10x10
+#define MRDMODE 0x71
+#define   MRDMODE_INTR_CH0  0x04
+#define   MRDMODE_INTR_CH1  0x08
+#define   MRDMODE_BLK_CH0   0x10
+#define   MRDMODE_BLK_CH1   0x20
+#define UDIDETCR0   0x73
+#define UDIDETCR1   0x7B
 
 static void cmd646_update_irq(PCIDevice *pd);
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index fd69ca3167..c2a2fab74c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -311,52 +311,52 @@ static void ide_cfata_identify(IDEState *s)
 
 cur_sec = s->cylinders * s->heads * s->sectors;
 
-put_le16(p + 0, 0x848a);   /* CF Storage Card signature */
-put_le16(p + 1, s->cylinders); /* Default cylinders */
-put_le16(p + 3, s->heads); /* Default heads */
-put_le16(p + 6, s->sectors);   /* Default sectors per track */
+put_le16(p + 0, 0x848a);/* CF Storage Card signature */
+put_le16(p + 1, s->cylinders);  /* Default cylinders */
+put_le16(p + 3, s->heads);  /* Default heads */
+put_le16(p + 6, s->sectors);/* Default sectors per track */
 /* *(p + 7) := nb_sectors >> 16 -- see ide_cfata_identify_size */
 /* *(p + 8) := nb_sectors   -- see ide_cfata_identify_size */
 padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
-put_le16(p + 22, 0x0004);  /* ECC bytes */
-padstr((char *) (p + 23), s->version, 8);  /* Firmware Revision */
+put_le16(p + 22, 0x0004);   /* ECC bytes */
+padstr((char *) (p + 23), s->version, 8);   /* Firmware Revision */
 padstr((char *) (p + 27), s->drive_model_str, 40);/* Model number */
 #if MAX_MULT_SECTORS > 1
 put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
 #else
 put_le16(p + 47, 0x);
 #endif
-put_le16(p + 49, 0x0f00);  /* Capabilities */
-put_le16(p + 51, 0x0002);  /* PIO cycle timing mode */
-put_le16(p + 52, 0x0001);  /* DMA cycle timing mode */
-put_le16(p + 53, 0x0003);  /* Translation params valid */
-put_le16(p + 54, s->cylinders);/* Current cylinders */
-put_le16(p + 55, s->heads);/* Current heads */
-put_le16(p + 56, s->sectors);  /* Current sectors */
-put_le16(p + 57, cur_sec); /* Current capacity */
-put_le16(p + 58, cur_sec >> 16);   /* Current capacity */
-if (s->mult_sectors)   /* Multiple sector setting */
+put_le16(p + 49, 0x0f00);   /* Capabilities */
+put_le16(p + 51, 0x0002);   /* PIO cycle timing mode */
+put_le16(p + 52, 0x0001);   /* DMA cycle timing mode */
+put_le16(p + 53, 0x0003);   /* Translation params valid */
+put_le16(p + 54, s->cylinders); /* Current cylinders */
+put_le16(p + 55, s->heads); /* Current heads */
+put_le16(p + 56, s->sectors);   /* Current sectors */
+put_le16(p + 57, cur_sec);  /* Current capacity */
+put_le16(p + 58, cur_sec >> 16);/* Current capacity */
+if (s->mult_sectors)/* Multiple sector setting */
 put_le16(p + 59, 0x100 | s->mult_sectors);
 /* *(p + 60) := nb_sectors   -- see ide_cfata_identify_size */
 /* *(p + 61) := nb_sectors >> 16 -- see ide_cfata_identify_size */
-put_le16(p + 63, 0x0203);  /* Multiword DMA capability */
-put_le16(p + 64, 0x0001);  /* Flow Control PIO support */
-put_le16(p + 65, 0x0096);  /* Min. Multiword DMA cycle */
-put_le16(p + 66, 0x0096);  /* Rec. Multiword DMA cycle */
-put_le16(p + 68, 0x00b4);  /* Min. PIO cycle time */
-put_le16(p + 82,

[PATCH qemu v2] QEMU coding style mandates spaces for indentation. This change replaces TABs in block files.

2021-09-29 Thread ~farzon 
From: Farzon Lotfi 

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371

Signed-off-by: Farzon Lotfi 
---
 block/bochs.c   | 10 +-
 block/file-posix.c  |  8 
 block/file-win32.c  | 20 ++--
 block/parallels.c   | 10 +-
 block/qcow.c| 10 +-
 include/block/nbd.h |  2 +-
 6 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/block/bochs.c b/block/bochs.c
index 2f010ab40a..01b84625c0 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -293,14 +293,14 @@ static void bochs_close(BlockDriverState *bs)
 }
 
 static BlockDriver bdrv_bochs = {
-.format_name   = "bochs",
-.instance_size = sizeof(BDRVBochsState),
-.bdrv_probe= bochs_probe,
-.bdrv_open = bochs_open,
+.format_name= "bochs",
+.instance_size  = sizeof(BDRVBochsState),
+.bdrv_probe = bochs_probe,
+.bdrv_open  = bochs_open,
 .bdrv_child_perm = bdrv_default_perms,
 .bdrv_refresh_limits = bochs_refresh_limits,
 .bdrv_co_preadv = bochs_co_preadv,
-.bdrv_close= bochs_close,
+.bdrv_close = bochs_close,
 .is_format  = true,
 };
 
diff --git a/block/file-posix.c b/block/file-posix.c
index d81e15efa4..9fc065506d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -127,7 +127,7 @@
 #define FTYPE_FILE   0
 #define FTYPE_CD 1
 
-#define MAX_BLOCKSIZE  4096
+#define MAX_BLOCKSIZE   4096
 
 /* Posix file locking bytes. Libvirt takes byte 0, we start from higher bytes,
  * leaving a few more bytes for its future use. */
@@ -3647,7 +3647,7 @@ static BlockDriver bdrv_host_device = {
 .bdrv_attach_aio_context = raw_aio_attach_aio_context,
 
 .bdrv_co_truncate   = raw_co_truncate,
-.bdrv_getlength= raw_getlength,
+.bdrv_getlength = raw_getlength,
 .bdrv_get_info = raw_get_info,
 .bdrv_get_allocated_file_size
 = raw_get_allocated_file_size,
@@ -3750,7 +3750,7 @@ static BlockDriver bdrv_host_cdrom = {
 .protocol_name  = "host_cdrom",
 .instance_size  = sizeof(BDRVRawState),
 .bdrv_needs_filename = true,
-.bdrv_probe_device = cdrom_probe_device,
+.bdrv_probe_device  = cdrom_probe_device,
 .bdrv_parse_filename = cdrom_parse_filename,
 .bdrv_file_open = cdrom_open,
 .bdrv_close = raw_close,
@@ -3881,7 +3881,7 @@ static BlockDriver bdrv_host_cdrom = {
 .protocol_name  = "host_cdrom",
 .instance_size  = sizeof(BDRVRawState),
 .bdrv_needs_filename = true,
-.bdrv_probe_device = cdrom_probe_device,
+.bdrv_probe_device  = cdrom_probe_device,
 .bdrv_parse_filename = cdrom_parse_filename,
 .bdrv_file_open = cdrom_open,
 .bdrv_close = raw_close,
diff --git a/block/file-win32.c b/block/file-win32.c
index b97c58d642..f80e62faf1 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -743,9 +743,9 @@ static QemuOptsList raw_create_opts = {
 };
 
 BlockDriver bdrv_file = {
-.format_name   = "file",
-.protocol_name = "file",
-.instance_size = sizeof(BDRVRawState),
+.format_name= "file",
+.protocol_name  = "file",
+.instance_size  = sizeof(BDRVRawState),
 .bdrv_needs_filename = true,
 .bdrv_parse_filename = raw_parse_filename,
 .bdrv_file_open = raw_open,
@@ -763,7 +763,7 @@ BlockDriver bdrv_file = {
 .bdrv_aio_flush = raw_aio_flush,
 
 .bdrv_co_truncate   = raw_co_truncate,
-.bdrv_getlength= raw_getlength,
+.bdrv_getlength = raw_getlength,
 .bdrv_get_allocated_file_size
 = raw_get_allocated_file_size,
 
@@ -915,14 +915,14 @@ done:
 }
 
 static BlockDriver bdrv_host_device = {
-.format_name   = "host_device",
-.protocol_name = "host_device",
-.instance_size = sizeof(BDRVRawState),
+.format_name= "host_device",
+.protocol_name  = "host_device",
+.instance_size  = sizeof(BDRVRawState),
 .bdrv_needs_filename = true,
 .bdrv_parse_filename = hdev_parse_filename,
-.bdrv_probe_device = hdev_probe_device,
-.bdrv_file_open= hdev_open,
-.bdrv_close= raw_close,
+.bdrv_probe_device  = hdev_probe_device,
+.bdrv_file_open = hdev_open,
+.bdrv_close = raw_close,
 .bdrv_refresh_limits = hdev_refresh_limits,
 
 .bdrv_aio_preadv= raw_aio_preadv,
diff --git a/block/parallels.c b/block/parallels.c
index 6ebad2a2bb..629d8aae2b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -916,11 +916,11 @@ static void parallels_close(BlockDriverState *bs)
 }
 
 static BlockDriver bdrv_parallels = {
-.format_name   = "parallels",
-.instance_size = sizeof(BDRVParallelsState),
-.bdrv_probe= parallels_probe,
-.bdrv_open = parallels_open,
-.bdrv_close= parallels_close,
+.format_name= "parallels",
+.instance_size  = sizeof(BDRVParallelsState),
+

Re: [PATCH 0/4] qemu-img compare --stat

2021-09-29 Thread John Snow 
On Wed, Sep 29, 2021 at 9:34 AM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:

> Hi all!
>
> Recently we faced the following task:
>
> Customer comes and say: incremental backup images are too fat. Does you
> incremental backup works correct?
>
> What to answer? We should check something. At least check that
> incremental images doesn't store same data twice. And we don't have a
> tool for it. I just wrote a simple python script to compare raw files
> cluster-by-cluster. Then we've mounted the qcow2 images with help of
> qemu-nbd, the resulting /dev/nbd* were compared and we proved that
> incremental backups don't store same data.
>
>
Good idea. I love diagnostic tools!


> But that leads to idea that some kind of that script would be good to
> have at hand.
>
> So, here is a new option for qemu-img compare, that is a lot more
> powerful and effective than original script, and allows to compare and
> calculate statistics, i.e. how many clusters differs, how many
> clusters changed from unallocated to data, and so on.
>
> For examples of output look at the test in patch 04.
>
> Vladimir Sementsov-Ogievskiy (4):
>   qemu-img: implement compare --stat
>   qemu-img: make --block-size optional for compare --stat
>   qemu-img: add --shallow option for qemu-img compare --stat
>   iotests: add qemu-img-compare-stat test
>
>  docs/tools/qemu-img.rst   |  29 +-
>  qemu-img.c| 275 +-
>  qemu-img-cmds.hx  |   4 +-
>  .../qemu-iotests/tests/qemu-img-compare-stat  |  88 ++
>

And new tests! :-)


>  .../tests/qemu-img-compare-stat.out   | 106 +++
>  5 files changed, 484 insertions(+), 18 deletions(-)
>  create mode 100755 tests/qemu-iotests/tests/qemu-img-compare-stat
>  create mode 100644 tests/qemu-iotests/tests/qemu-img-compare-stat.out
>
>
>

Re: [PATCH 3/4] qemu-img: add --shallow option for qemu-img compare --stat

2021-09-29 Thread Nir Soffer 
On Wed, Sep 29, 2021 at 7:28 PM Vladimir Sementsov-Ogievskiy
 wrote:
>
> 29.09.2021 19:00, Nir Soffer wrote:
> > On Wed, Sep 29, 2021 at 4:37 PM Vladimir Sementsov-Ogievskiy
> >  wrote:
> >>
> >> Allow compare only top images of backing chains. That's useful for
> >> comparing two increments from the same chain of incremental backups.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> >> ---
> >>   docs/tools/qemu-img.rst |  8 +++-
> >>   qemu-img.c  | 14 --
> >>   qemu-img-cmds.hx|  4 ++--
> >>   3 files changed, 21 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> >> index 4b382ca2b0..c8ae96be6a 100644
> >> --- a/docs/tools/qemu-img.rst
> >> +++ b/docs/tools/qemu-img.rst
> >> @@ -176,6 +176,12 @@ Parameters to compare subcommand:
> >>   - If both files don't specify cluster-size, use default of 64K
> >>   - If only one file specify cluster-size, just use it.
> >>
> >> +.. option:: --shallow
> >
> > We use the same term in oVirt when we upload/download one layer from a 
> > chain.
> >
> >> +  Only allowed with ``--stat``. This option prevents opening and comparing
> >> +  any backing files. This is useful to compare incremental images from
> >> +  the chain of incremental backups.
> >
> > This is useful also without --stat. Our current workaround in oVirt is
> > to use unsafe
> > rebase to disconnect the top image from the base image so we can compare
> > source and destination image after backup.
> >
> > Here is an example of test code that could use --shallow (regardless of 
> > --stat):
> > https://github.com/oVirt/ovirt-imageio/blob/master/daemon/test/backup_test.py#L114
> >
> > Do you have any reason to limit --shallow to --stats?
>
>
> Hmm. I wrongly thought that without --stat qemu-img compare will fail on 
> first mismatch, which will occur soon, as we don't have backing images and 
> it's just superfluous.
>
> But actually, qemu-img will not compare "unallocated" areas.
>
> Ok, I agree, in v2 I'll allow --shallow without --stat.
>
>
> Another question to discuss: we already have "-u" option in qemu-img create 
> and qemu-img rebase to not open backing files. And 'u' means 'unsafe'.
> I don't think that "unsafe" term is good for qemu-img compare --stat, that's 
> why I decided to call it differently: "shallow".
> Still for qemu-img compare (without --stat) "unsafe" term make sense.
>
>
> So, it probably better to follow common notation, and call the option "-u".

--shallow is better, comparing a single image from a chain is a safe operation.
Replacing a backing file or creating an image on top of one without checking
the backing file is not.

>
> >
> >> +
> >>   Parameters to convert subcommand:
> >>
> >>   .. program:: qemu-img-convert
> >> @@ -395,7 +401,7 @@ Command description:
> >>
> >> The rate limit for the commit process is specified by ``-r``.
> >>
> >> -.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] 
> >> [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] 
> >> FILENAME1 FILENAME2
> >> +.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] 
> >> [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] 
> >> [--shallow]] FILENAME1 FILENAME2
> >>
> >> Check if two images have the same content. You can compare images with
> >> different format or settings.
> >> diff --git a/qemu-img.c b/qemu-img.c
> >> index 61e7f470bb..e8ae412c38 100644
> >> --- a/qemu-img.c
> >> +++ b/qemu-img.c
> >> @@ -85,6 +85,7 @@ enum {
> >>   OPTION_SKIP_BROKEN = 277,
> >>   OPTION_STAT = 277,
> >>   OPTION_BLOCK_SIZE = 278,
> >> +OPTION_SHALLOW = 279,
> >>   };
> >>
> >>   typedef enum OutputFormat {
> >> @@ -1482,7 +1483,7 @@ static int img_compare(int argc, char **argv)
> >>   int64_t block_end;
> >>   int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */
> >>   bool progress = false, quiet = false, strict = false;
> >> -int flags;
> >> +int flags = 0;
> >>   bool writethrough;
> >>   int64_t total_size;
> >>   int64_t offset = 0;
> >> @@ -1504,6 +1505,7 @@ static int img_compare(int argc, char **argv)
> >>   {"force-share", no_argument, 0, 'U'},
> >>   {"stat", no_argument, 0, OPTION_STAT},
> >>   {"block-size", required_argument, 0, OPTION_BLOCK_SIZE},
> >> +{"shallow", no_argument, 0, OPTION_SHALLOW},
> >>   {0, 0, 0, 0}
> >>   };
> >>   c = getopt_long(argc, argv, ":hf:F:T:pqsU",
> >> @@ -1569,6 +1571,9 @@ static int img_compare(int argc, char **argv)
> >>   exit(EXIT_SUCCESS);
> >>   }
> >>   break;
> >> +case OPTION_SHALLOW:
> >> +flags |= BDRV_O_NO_BACKING;
> >> +break;
> >>   }
> >>   }
> >>
> >> @@ -1590,10 +1595,15 @@ static int img_compare(int argc, char **argv)
> >>   goto out;
> >>

Re: [PATCH 3/4] qemu-img: add --shallow option for qemu-img compare --stat

2021-09-29 Thread Vladimir Sementsov-Ogievskiy 

29.09.2021 19:00, Nir Soffer wrote:

On Wed, Sep 29, 2021 at 4:37 PM Vladimir Sementsov-Ogievskiy
 wrote:


Allow compare only top images of backing chains. That's useful for
comparing two increments from the same chain of incremental backups.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  docs/tools/qemu-img.rst |  8 +++-
  qemu-img.c  | 14 --
  qemu-img-cmds.hx|  4 ++--
  3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 4b382ca2b0..c8ae96be6a 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -176,6 +176,12 @@ Parameters to compare subcommand:
  - If both files don't specify cluster-size, use default of 64K
  - If only one file specify cluster-size, just use it.

+.. option:: --shallow


We use the same term in oVirt when we upload/download one layer from a chain.


+  Only allowed with ``--stat``. This option prevents opening and comparing
+  any backing files. This is useful to compare incremental images from
+  the chain of incremental backups.


This is useful also without --stat. Our current workaround in oVirt is
to use unsafe
rebase to disconnect the top image from the base image so we can compare
source and destination image after backup.

Here is an example of test code that could use --shallow (regardless of --stat):
https://github.com/oVirt/ovirt-imageio/blob/master/daemon/test/backup_test.py#L114

Do you have any reason to limit --shallow to --stats?



Hmm. I wrongly thought that without --stat qemu-img compare will fail on first 
mismatch, which will occur soon, as we don't have backing images and it's just 
superfluous.

But actually, qemu-img will not compare "unallocated" areas.

Ok, I agree, in v2 I'll allow --shallow without --stat.


Another question to discuss: we already have "-u" option in qemu-img create and 
qemu-img rebase to not open backing files. And 'u' means 'unsafe'.
I don't think that "unsafe" term is good for qemu-img compare --stat, that's why I 
decided to call it differently: "shallow".
Still for qemu-img compare (without --stat) "unsafe" term make sense.


So, it probably better to follow common notation, and call the option "-u".





+
  Parameters to convert subcommand:

  .. program:: qemu-img-convert
@@ -395,7 +401,7 @@ Command description:

The rate limit for the commit process is specified by ``-r``.

-.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] FILENAME1 
FILENAME2
+.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] [--shallow]] 
FILENAME1 FILENAME2

Check if two images have the same content. You can compare images with
different format or settings.
diff --git a/qemu-img.c b/qemu-img.c
index 61e7f470bb..e8ae412c38 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -85,6 +85,7 @@ enum {
  OPTION_SKIP_BROKEN = 277,
  OPTION_STAT = 277,
  OPTION_BLOCK_SIZE = 278,
+OPTION_SHALLOW = 279,
  };

  typedef enum OutputFormat {
@@ -1482,7 +1483,7 @@ static int img_compare(int argc, char **argv)
  int64_t block_end;
  int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */
  bool progress = false, quiet = false, strict = false;
-int flags;
+int flags = 0;
  bool writethrough;
  int64_t total_size;
  int64_t offset = 0;
@@ -1504,6 +1505,7 @@ static int img_compare(int argc, char **argv)
  {"force-share", no_argument, 0, 'U'},
  {"stat", no_argument, 0, OPTION_STAT},
  {"block-size", required_argument, 0, OPTION_BLOCK_SIZE},
+{"shallow", no_argument, 0, OPTION_SHALLOW},
  {0, 0, 0, 0}
  };
  c = getopt_long(argc, argv, ":hf:F:T:pqsU",
@@ -1569,6 +1571,9 @@ static int img_compare(int argc, char **argv)
  exit(EXIT_SUCCESS);
  }
  break;
+case OPTION_SHALLOW:
+flags |= BDRV_O_NO_BACKING;
+break;
  }
  }

@@ -1590,10 +1595,15 @@ static int img_compare(int argc, char **argv)
  goto out;
  }

+if (!do_stat && (flags & BDRV_O_NO_BACKING)) {
+error_report("--shallow can be used only together with --stat");
+ret = 1;
+goto out;
+}
+
  /* Initialize before goto out */
  qemu_progress_init(progress, 2.0);

-flags = 0;
  ret = bdrv_parse_cache_mode(cache, , );
  if (ret < 0) {
  error_report("Invalid source cache option: %s", cache);
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 96a193eea8..a295bc6860 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -40,9 +40,9 @@ SRST
  ERST

  DEF("compare", img_compare,
-"compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] 
[-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]]

Re: [PATCH 3/4] qemu-img: add --shallow option for qemu-img compare --stat

2021-09-29 Thread Nir Soffer 
On Wed, Sep 29, 2021 at 4:37 PM Vladimir Sementsov-Ogievskiy
 wrote:
>
> Allow compare only top images of backing chains. That's useful for
> comparing two increments from the same chain of incremental backups.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  docs/tools/qemu-img.rst |  8 +++-
>  qemu-img.c  | 14 --
>  qemu-img-cmds.hx|  4 ++--
>  3 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 4b382ca2b0..c8ae96be6a 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -176,6 +176,12 @@ Parameters to compare subcommand:
>  - If both files don't specify cluster-size, use default of 64K
>  - If only one file specify cluster-size, just use it.
>
> +.. option:: --shallow

We use the same term in oVirt when we upload/download one layer from a chain.

> +  Only allowed with ``--stat``. This option prevents opening and comparing
> +  any backing files. This is useful to compare incremental images from
> +  the chain of incremental backups.

This is useful also without --stat. Our current workaround in oVirt is
to use unsafe
rebase to disconnect the top image from the base image so we can compare
source and destination image after backup.

Here is an example of test code that could use --shallow (regardless of --stat):
https://github.com/oVirt/ovirt-imageio/blob/master/daemon/test/backup_test.py#L114

Do you have any reason to limit --shallow to --stats?

> +
>  Parameters to convert subcommand:
>
>  .. program:: qemu-img-convert
> @@ -395,7 +401,7 @@ Command description:
>
>The rate limit for the commit process is specified by ``-r``.
>
> -.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] 
> [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] 
> FILENAME1 FILENAME2
> +.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] 
> [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] 
> [--shallow]] FILENAME1 FILENAME2
>
>Check if two images have the same content. You can compare images with
>different format or settings.
> diff --git a/qemu-img.c b/qemu-img.c
> index 61e7f470bb..e8ae412c38 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -85,6 +85,7 @@ enum {
>  OPTION_SKIP_BROKEN = 277,
>  OPTION_STAT = 277,
>  OPTION_BLOCK_SIZE = 278,
> +OPTION_SHALLOW = 279,
>  };
>
>  typedef enum OutputFormat {
> @@ -1482,7 +1483,7 @@ static int img_compare(int argc, char **argv)
>  int64_t block_end;
>  int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */
>  bool progress = false, quiet = false, strict = false;
> -int flags;
> +int flags = 0;
>  bool writethrough;
>  int64_t total_size;
>  int64_t offset = 0;
> @@ -1504,6 +1505,7 @@ static int img_compare(int argc, char **argv)
>  {"force-share", no_argument, 0, 'U'},
>  {"stat", no_argument, 0, OPTION_STAT},
>  {"block-size", required_argument, 0, OPTION_BLOCK_SIZE},
> +{"shallow", no_argument, 0, OPTION_SHALLOW},
>  {0, 0, 0, 0}
>  };
>  c = getopt_long(argc, argv, ":hf:F:T:pqsU",
> @@ -1569,6 +1571,9 @@ static int img_compare(int argc, char **argv)
>  exit(EXIT_SUCCESS);
>  }
>  break;
> +case OPTION_SHALLOW:
> +flags |= BDRV_O_NO_BACKING;
> +break;
>  }
>  }
>
> @@ -1590,10 +1595,15 @@ static int img_compare(int argc, char **argv)
>  goto out;
>  }
>
> +if (!do_stat && (flags & BDRV_O_NO_BACKING)) {
> +error_report("--shallow can be used only together with --stat");
> +ret = 1;
> +goto out;
> +}
> +
>  /* Initialize before goto out */
>  qemu_progress_init(progress, 2.0);
>
> -flags = 0;
>  ret = bdrv_parse_cache_mode(cache, , );
>  if (ret < 0) {
>  error_report("Invalid source cache option: %s", cache);
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 96a193eea8..a295bc6860 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -40,9 +40,9 @@ SRST
>  ERST
>
>  DEF("compare", img_compare,
> -"compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T 
> src_cache] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] filename1 
> filename2")
> +"compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T 
> src_cache] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] [--shallow]] 
> filename1 filename2")
>  SRST
> -.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] 
> [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] 
> FILENAME1 FILENAME2
> +.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] 
> [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] 
> [--shallow]] FILENAME1 FILENAME2
>  ERST
>
>  DEF("convert", img_convert,
> --

[PATCH 4/4] iotests: add qemu-img-compare-stat test

2021-09-29 Thread Vladimir Sementsov-Ogievskiy 
Test new feature qemu-img compare --stat.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 .../qemu-iotests/tests/qemu-img-compare-stat  |  88 +++
 .../tests/qemu-img-compare-stat.out   | 106 ++
 2 files changed, 194 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/qemu-img-compare-stat
 create mode 100644 tests/qemu-iotests/tests/qemu-img-compare-stat.out

diff --git a/tests/qemu-iotests/tests/qemu-img-compare-stat 
b/tests/qemu-iotests/tests/qemu-img-compare-stat
new file mode 100755
index 00..e2c0bcc7ef
--- /dev/null
+++ b/tests/qemu-iotests/tests/qemu-img-compare-stat
@@ -0,0 +1,88 @@
+#!/usr/bin/env python3
+#
+# Test qemu-img compare --stat
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+from iotests import qemu_img_create, qemu_io, qemu_img_log, log
+
+iotests.script_initialize(supported_fmts=['qcow2'])
+
+a, b, c = iotests.file_path('a', 'b', 'c')
+
+log('= compare two images =\n')
+
+qemu_img_create('-f', iotests.imgfmt, a, '1M')
+qemu_img_create('-f', iotests.imgfmt, b, '1M')
+
+# equal data and zero
+qemu_io('-c', 'write -z 0 64K', a)
+qemu_io('-c', 'write -P 0 0 64K', b)
+
+# different data
+qemu_io('-c', 'write -P 1 64K 64K', a)
+qemu_io('-c', 'write -P 0 64K 64K', b)
+
+# equal data
+qemu_io('-c', 'write -P 2 128K 64K', a)
+qemu_io('-c', 'write -P 2 128K 64K', b)
+
+# equal unallocated and allocated zero
+qemu_io('-c', 'write -z 192K 64K', b)
+
+# unequal data and unallocated zero
+qemu_io('-c', 'write -P 3 256K 64K', a)
+
+qemu_img_log('compare', '--stat', a, b)
+
+log('\n= compare two increments =\n')
+
+qemu_img_create('-f', iotests.imgfmt, a, '1M')
+qemu_img_create('-f', iotests.imgfmt, '-b', a, '-F', iotests.imgfmt, b, '1M')
+qemu_img_create('-f', iotests.imgfmt, '-b', b, '-F', iotests.imgfmt, c, '1M')
+
+qemu_io('-c', 'write -P 1 0 1M', a)
+qemu_io('-c', 'write -P 2 0 64K', b)
+qemu_io('-c', 'write -P 3 64K 64K', c)
+qemu_img_log('compare', '--stat', b, c)
+
+log('\n= compare two increments with --shallow=\n')
+qemu_img_log('compare', '--stat', '--shallow', b, c)
+
+log('\n= compare images of different size =\n')
+qemu_img_create('-f', iotests.imgfmt, a, '1M')
+qemu_img_create('-f', iotests.imgfmt, b, '2M')
+qemu_io('-c', 'write -P 1 0 1M', a)
+qemu_io('-c', 'write -P 2 0 1M', b)
+qemu_io('-c', 'write -P 1 1M 64K', b)
+qemu_io('-c', f'write -z {1024 + 64 * 2}K 64K', b)
+qemu_io('-c', f'write -P 0 {1024 + 64 * 3}K 64K', b)
+qemu_img_log('compare', '--stat', a, b)
+
+log('\n= compare images with only 512 bytes different =\n')
+qemu_img_create('-f', iotests.imgfmt, a, '1M')
+qemu_img_create('-f', iotests.imgfmt, b, '1M')
+qemu_io('-c', 'write -P 1 0 1M', a)
+qemu_io('-c', 'write -P 2 0 512', b)
+qemu_io('-c', f'write -P 1 512 {1024 * 1024 - 512}', b)
+qemu_img_log('compare', '--stat', a, b)
+
+log('\n= compare images with only 512 bytes different, block-size=4K =\n')
+qemu_img_log('compare', '--stat', '--block-size', '4K', a, b)
+
+log('\n= end =')
diff --git a/tests/qemu-iotests/tests/qemu-img-compare-stat.out 
b/tests/qemu-iotests/tests/qemu-img-compare-stat.out
new file mode 100644
index 00..0dec76feb6
--- /dev/null
+++ b/tests/qemu-iotests/tests/qemu-img-compare-stat.out
@@ -0,0 +1,106 @@
+= compare two images =
+
+Compare stats:
+Agenda
+D: DATA
+Z: ZERO
+A: ALLOCATED
+E: after end of file
+
+Equal:
+_Z__ -> _Z__ 720896 bytes (704 KiB) 68.8%
+_Z__ -> _ZA_ 65536 bytes (64 KiB) 6.2%
+D_A_ -> D_A_ 65536 bytes (64 KiB) 6.2%
+_ZA_ -> D_A_ 65536 bytes (64 KiB) 6.2%
+
+Unequal:
+D_A_ -> _Z__ 65536 bytes (64 KiB) 6.2%
+D_A_ -> D_A_ 65536 bytes (64 KiB) 6.2%
+
+
+= compare two increments =
+
+Compare stats:
+Agenda
+D: DATA
+Z: ZERO
+A: ALLOCATED
+E: after end of file
+
+Equal:
+D_A_ -> D_A_ 983040 bytes (960 KiB) 93.8%
+
+Unequal:
+D_A_ -> D_A_ 65536 bytes (64 KiB) 6.2%
+
+
+= compare two increments with --shallow=
+
+Compare stats:
+Agenda
+D: DATA
+Z: ZERO
+A: ALLOCATED
+E: after end of file
+
+Equal:
+_Z__ -> _Z__ 917504 bytes (896 KiB) 87.5%
+
+Unequal:
+_Z__ -> D_A_ 65536 bytes (64 KiB) 6.2%
+D_A_ -> _Z__ 65536 bytes (64 KiB) 6.2%
+
+
+= compare images of different size =
+
+Warning: Image size mismatch!
+Compare stats:
+Agenda
+D: DATA
+Z: ZERO
+A: ALLOCATED
+E: after end of file
+
+Equal:
+_Z_E -> _Z__ 851968 bytes (832 KiB) 40.6%
+_Z_E -> D_A_ 65536 bytes

[PATCH 1/4] qemu-img: implement compare --stat

2021-09-29 Thread Vladimir Sementsov-Ogievskiy 
With new option qemu-img compare will not stop at first mismatch, but
instead calculate statistics: how many clusters with different data,
how many clusters with equal data, how many clusters were unallocated
but become data and so on.

We compare images chunk by chunk. Chunk size depends on what
block_status returns for both images. It may return less than cluster
(remember about qcow2 subclusters), it may return more than cluster (if
several consecutive clusters share same status). Finally images may
have different cluster sizes. This all leads to ambiguity in how to
finally compare the data.

What we can say for sure is that, when we compare two qcow2 images with
same cluster size, we should compare clusters with data separately.
Otherwise, if we for example compare 10 consecutive clusters of data
where only one byte differs we'll report 10 different clusters.
Expected result in this case is 1 different cluster and 9 equal ones.

So, to serve this case and just to have some defined rule let's do the
following:

1. Select some block-size for compare procedure. In this commit it must
   be specified by user, next commit will add some automatic logic and
   make --block-size optional.

2. Go chunk-by-chunk using block_status as we do now with only one
   differency:
   If block_status() returns DATA region that intersects block-size
   aligned boundary, crop this region at this boundary.

This way it's still possible to compare less than cluster and report
subcluster-level accuracy, but we newer compare more than one cluster
of data.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 docs/tools/qemu-img.rst |  18 +++-
 qemu-img.c  | 206 +---
 qemu-img-cmds.hx|   4 +-
 3 files changed, 212 insertions(+), 16 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index d58980aef8..21164253d4 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -159,6 +159,18 @@ Parameters to compare subcommand:
 
   Strict mode - fail on different image size or sector allocation
 
+.. option:: --stat
+
+  Instead of exit on first mismatch compare the whole images and print
+  statistics on amount of different pairs of clusters, based on their
+  block-status and are they equal or not.
+
+.. option:: --block-size BLOCK_SIZE
+
+  Block size for comparing with ``--stat``. This doesn't guarantee exact
+  size of comparing chunks, but that guarantee that data chunks being
+  compared will never cross aligned block-size boundary.
+
 Parameters to convert subcommand:
 
 .. program:: qemu-img-convert
@@ -378,7 +390,7 @@ Command description:
 
   The rate limit for the commit process is specified by ``-r``.
 
-.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] FILENAME1 FILENAME2
+.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] [--stat --block-size BLOCK_SIZE] FILENAME1 
FILENAME2
 
   Check if two images have the same content. You can compare images with
   different format or settings.
@@ -405,9 +417,9 @@ Command description:
   The following table sumarizes all exit codes of the compare subcommand:
 
   0
-Images are identical (or requested help was printed)
+Images are identical (or requested help was printed, or ``--stat`` was 
used)
   1
-Images differ
+Images differ (1 is never returned when ``--stat`` option specified)
   2
 Error on opening an image
   3
diff --git a/qemu-img.c b/qemu-img.c
index f036a1d428..79a0589167 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -83,6 +83,8 @@ enum {
 OPTION_BITMAPS = 275,
 OPTION_FORCE = 276,
 OPTION_SKIP_BROKEN = 277,
+OPTION_STAT = 277,
+OPTION_BLOCK_SIZE = 278,
 };
 
 typedef enum OutputFormat {
@@ -1304,6 +1306,107 @@ static int check_empty_sectors(BlockBackend *blk, 
int64_t offset,
 return 0;
 }
 
+#define IMG_CMP_STATUS_MASK (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | \
+ BDRV_BLOCK_ALLOCATED)
+#define IMG_CMP_STATUS_MAX (IMG_CMP_STATUS_MASK | BDRV_BLOCK_EOF)
+
+typedef struct ImgCmpStat {
+/* stat: [ret: 0 is equal, 1 is not][status1][status2] -> n_bytes */
+uint64_t stat[2][IMG_CMP_STATUS_MAX + 1][IMG_CMP_STATUS_MAX + 1];
+} ImgCmpStat;
+
+/*
+ * To denote chunks after EOF when compared files are of different size, use
+ * corresponding status = -1
+ */
+static void cmp_stat_account(ImgCmpStat *stat, bool differs,
+ int status1, int status2, int64_t bytes)
+{
+assert(status1 >= -1);
+if (status1 == -1) {
+/*
+ * Note BDRV_BLOCK_EOF: we abuse it here a bit. User is not interested
+ * in EOF flag in the last chunk of file (especially when we trim the
+ * chunk and EOF becomes incorrect), so BDRV_BLOCK_EOF is not in
+ * IMG_CMP_STATUS_MASK. We instead use BDRV_BLOCK_EOF to illustrate
+ * chunks after-end-of-file in

[PATCH 3/4] qemu-img: add --shallow option for qemu-img compare --stat

2021-09-29 Thread Vladimir Sementsov-Ogievskiy 
Allow compare only top images of backing chains. That's useful for
comparing two increments from the same chain of incremental backups.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 docs/tools/qemu-img.rst |  8 +++-
 qemu-img.c  | 14 --
 qemu-img-cmds.hx|  4 ++--
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 4b382ca2b0..c8ae96be6a 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -176,6 +176,12 @@ Parameters to compare subcommand:
 - If both files don't specify cluster-size, use default of 64K
 - If only one file specify cluster-size, just use it.
 
+.. option:: --shallow
+
+  Only allowed with ``--stat``. This option prevents opening and comparing
+  any backing files. This is useful to compare incremental images from
+  the chain of incremental backups.
+
 Parameters to convert subcommand:
 
 .. program:: qemu-img-convert
@@ -395,7 +401,7 @@ Command description:
 
   The rate limit for the commit process is specified by ``-r``.
 
-.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] FILENAME1 
FILENAME2
+.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] [--shallow]] 
FILENAME1 FILENAME2
 
   Check if two images have the same content. You can compare images with
   different format or settings.
diff --git a/qemu-img.c b/qemu-img.c
index 61e7f470bb..e8ae412c38 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -85,6 +85,7 @@ enum {
 OPTION_SKIP_BROKEN = 277,
 OPTION_STAT = 277,
 OPTION_BLOCK_SIZE = 278,
+OPTION_SHALLOW = 279,
 };
 
 typedef enum OutputFormat {
@@ -1482,7 +1483,7 @@ static int img_compare(int argc, char **argv)
 int64_t block_end;
 int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */
 bool progress = false, quiet = false, strict = false;
-int flags;
+int flags = 0;
 bool writethrough;
 int64_t total_size;
 int64_t offset = 0;
@@ -1504,6 +1505,7 @@ static int img_compare(int argc, char **argv)
 {"force-share", no_argument, 0, 'U'},
 {"stat", no_argument, 0, OPTION_STAT},
 {"block-size", required_argument, 0, OPTION_BLOCK_SIZE},
+{"shallow", no_argument, 0, OPTION_SHALLOW},
 {0, 0, 0, 0}
 };
 c = getopt_long(argc, argv, ":hf:F:T:pqsU",
@@ -1569,6 +1571,9 @@ static int img_compare(int argc, char **argv)
 exit(EXIT_SUCCESS);
 }
 break;
+case OPTION_SHALLOW:
+flags |= BDRV_O_NO_BACKING;
+break;
 }
 }
 
@@ -1590,10 +1595,15 @@ static int img_compare(int argc, char **argv)
 goto out;
 }
 
+if (!do_stat && (flags & BDRV_O_NO_BACKING)) {
+error_report("--shallow can be used only together with --stat");
+ret = 1;
+goto out;
+}
+
 /* Initialize before goto out */
 qemu_progress_init(progress, 2.0);
 
-flags = 0;
 ret = bdrv_parse_cache_mode(cache, , );
 if (ret < 0) {
 error_report("Invalid source cache option: %s", cache);
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 96a193eea8..a295bc6860 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -40,9 +40,9 @@ SRST
 ERST
 
 DEF("compare", img_compare,
-"compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T 
src_cache] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] filename1 
filename2")
+"compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T 
src_cache] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] [--shallow]] 
filename1 filename2")
 SRST
-.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] FILENAME1 
FILENAME2
+.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE] [--shallow]] 
FILENAME1 FILENAME2
 ERST
 
 DEF("convert", img_convert,
-- 
2.29.2

[PATCH 0/4] qemu-img compare --stat

2021-09-29 Thread Vladimir Sementsov-Ogievskiy 
Hi all!

Recently we faced the following task:

Customer comes and say: incremental backup images are too fat. Does you
incremental backup works correct?

What to answer? We should check something. At least check that
incremental images doesn't store same data twice. And we don't have a
tool for it. I just wrote a simple python script to compare raw files
cluster-by-cluster. Then we've mounted the qcow2 images with help of
qemu-nbd, the resulting /dev/nbd* were compared and we proved that
incremental backups don't store same data.

But that leads to idea that some kind of that script would be good to
have at hand.

So, here is a new option for qemu-img compare, that is a lot more
powerful and effective than original script, and allows to compare and
calculate statistics, i.e. how many clusters differs, how many
clusters changed from unallocated to data, and so on.

For examples of output look at the test in patch 04.

Vladimir Sementsov-Ogievskiy (4):
  qemu-img: implement compare --stat
  qemu-img: make --block-size optional for compare --stat
  qemu-img: add --shallow option for qemu-img compare --stat
  iotests: add qemu-img-compare-stat test

 docs/tools/qemu-img.rst   |  29 +-
 qemu-img.c| 275 +-
 qemu-img-cmds.hx  |   4 +-
 .../qemu-iotests/tests/qemu-img-compare-stat  |  88 ++
 .../tests/qemu-img-compare-stat.out   | 106 +++
 5 files changed, 484 insertions(+), 18 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/qemu-img-compare-stat
 create mode 100644 tests/qemu-iotests/tests/qemu-img-compare-stat.out

-- 
2.29.2

[PATCH 2/4] qemu-img: make --block-size optional for compare --stat

2021-09-29 Thread Vladimir Sementsov-Ogievskiy 
Let's detect block-size automatically if not specified by user:

 If both files define cluster-size, use minimum to be more precise.
 If both files don't specify cluster-size, use default of 64K
 If only one file specify cluster-size, just use it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 docs/tools/qemu-img.rst |  7 +++-
 qemu-img.c  | 71 -
 qemu-img-cmds.hx|  4 +--
 3 files changed, 71 insertions(+), 11 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 21164253d4..4b382ca2b0 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -170,6 +170,11 @@ Parameters to compare subcommand:
   Block size for comparing with ``--stat``. This doesn't guarantee exact
   size of comparing chunks, but that guarantee that data chunks being
   compared will never cross aligned block-size boundary.
+  When unspecified the following logic is used:
+
+- If both files define cluster-size, use minimum to be more precise.
+- If both files don't specify cluster-size, use default of 64K
+- If only one file specify cluster-size, just use it.
 
 Parameters to convert subcommand:
 
@@ -390,7 +395,7 @@ Command description:
 
   The rate limit for the commit process is specified by ``-r``.
 
-.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] [--stat --block-size BLOCK_SIZE] FILENAME1 
FILENAME2
+.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] FILENAME1 
FILENAME2
 
   Check if two images have the same content. You can compare images with
   different format or settings.
diff --git a/qemu-img.c b/qemu-img.c
index 79a0589167..61e7f470bb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1407,6 +1407,61 @@ static void cmp_stat_print(ImgCmpStat *stat, int64_t 
total_bytes)
 }
 }
 
+/* Get default value for qemu-img compare --block-size option. */
+static int img_compare_block_size(BlockDriverState *bs1,
+  BlockDriverState *bs2,
+  bool quiet)
+{
+const int default_block_size = 64 * 1024; /* 64K */
+
+int ret;
+BlockDriverInfo bdi;
+int cluster_size1, cluster_size2, block_size;
+const char *note = "Note: to alter it, set --block-size option.";
+const char *fname1 = bs1->filename;
+const char *fname2 = bs2->filename;
+
+ret = bdrv_get_info(bs1, );
+if (ret < 0 && ret != -ENOTSUP) {
+error_report("Failed to get info of %s: %s", fname1, strerror(-ret));
+return ret;
+}
+cluster_size1 = ret < 0 ? 0 : bdi.cluster_size;
+
+ret = bdrv_get_info(bs2, );
+if (ret < 0 && ret != -ENOTSUP) {
+error_report("Failed to get info of %s: %s", fname2, strerror(-ret));
+return ret;
+}
+cluster_size2 = ret < 0 ? 0 : bdi.cluster_size;
+
+if (cluster_size1 > 0 && cluster_size2 > 0) {
+if (cluster_size1 == cluster_size2) {
+block_size = cluster_size1;
+} else {
+block_size = MIN(cluster_size1, cluster_size2);
+qprintf(quiet, "%s and %s has different cluster sizes: %d and %d "
+"correspondingly. Use minimum as block-size for "
+"accuracy: %d. %s\n",
+fname1, fname2, cluster_size1,
+cluster_size2, block_size, note);
+}
+} else if (cluster_size1 == 0 && cluster_size2 == 0) {
+block_size = default_block_size;
+qprintf(quiet, "Neither of %s and %s has explicit cluster size. Use "
+"default of %d bytes. %s\n", fname1, fname2, block_size, note);
+} else {
+block_size = MAX(cluster_size1, cluster_size2);
+qprintf(quiet, "%s has explicit cluster size of %d and %s "
+"doesn't have one. Use %d as block-size. %s\n",
+cluster_size1 ? fname1 : fname2, block_size,
+cluster_size1 ? fname2 : fname1,
+block_size, note);
+}
+
+return block_size;
+}
+
 /*
  * Compares two images. Exit codes:
  *
@@ -1535,14 +1590,6 @@ static int img_compare(int argc, char **argv)
 goto out;
 }
 
-if (do_stat && !block_size) {
-/* TODO: make block-size optional */
-error_report("You must specify --block-size together with --stat");
-ret = 1;
-goto out;
-}
-
-
 /* Initialize before goto out */
 qemu_progress_init(progress, 2.0);
 
@@ -1589,6 +1636,14 @@ static int img_compare(int argc, char **argv)
 total_size = MIN(total_size1, total_size2);
 progress_base = MAX(total_size1, total_size2);
 
+if (do_stat && !block_size) {
+block_size = img_compare_block_size(bs1, bs2, quiet);
+if (block_size <= 0) {
+ret = 4;
+goto out;
+}
+}
+
 qemu_progress_print(0, 100);
 
 if (strict &&

Re: [PATCH qemu] issue 371: convert tabs to spaces for the block subsystem.

2021-09-29 Thread Thomas Huth 

On 29/09/2021 07.30, ~farzon wrote:

From: Farzon Lotfi 


 Hi!

Thanks for your contribution! However, there are some more rules that need 
to be followed to get a patch accepted in the QEMU project:


Please provide a proper patch description for your changes (something like: 
"QEMU coding style mandates spaces for indentation, so the TABs should get 
replaced in these files" or something similar should do it here).


Then it would be nice to have a "Resolves:" line in here, too:

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/371

That way, the issue gets closed automatically once the patch has been 
accepted. Then please also drop the "issue 371:" from the subject line.


See also:
https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message

And finally, the most important one: You need to supply a "Signed-off-by:" 
line at the end of the patch description to make it clear that your patch 
follows the Developer Certificat of Origin. See also:


https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line


  15 files changed, 639 insertions(+), 639 deletions(-)


I think the patch is rather big already. Could you please split it up in 
three patches instead:


- One for block/ and include/block
- One for hw/block/ and include/hw/block
- One for hw/ide/ and include/hw/ide

Please also make sure to CC: qemu-block@nongnu.org on these patches (I 
recommend to use the scripts/get_maintainer.pl script to get the right list 
of people to put into the CC: list of the patches).


 HTH,
  Thomas