Re: [PATCH v3 4/8] qemu-img: add chunk size parameter to compare_buffers()

[Kevin Wolf]

Am 19.09.2023 um 18:58 hat Andrey Drobyshev geschrieben:
> Add @chsize param to the function which, if non-zero, would represent
> the chunk size to be used for comparison.  If it's zero, then
> BDRV_SECTOR_SIZE is used as default chunk size, which is the previous
> behaviour.
> 
> In particular, we're going to use this param in img_rebase() to make the
> write requests aligned to a predefined alignment value.
> 
> Signed-off-by: Andrey Drobyshev 
> Reviewed-by: Eric Blake 
> Reviewed-by: Hanna Czenczek 
> ---
>  qemu-img.c | 24 +++-
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 4dc91505bf..0f67b021f7 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1274,23 +1274,29 @@ static int is_allocated_sectors_min(const uint8_t 
> *buf, int n, int *pnum,
>  }
>  
>  /*
> - * Compares two buffers sector by sector. Returns 0 if the first
> - * sector of each buffer matches, non-zero otherwise.
> + * Compares two buffers chunk by chunk, where @chsize is the chunk size.
> + * If @chsize is 0, default chunk size of BDRV_SECTOR_SIZE is used.
> + * Returns 0 if the first chunk of each buffer matches, non-zero otherwise.
>   *
> - * pnum is set to the sector-aligned size of the buffer prefix that
> - * has the same matching status as the first sector.
> + * @pnum is set to the size of the buffer prefix aligned to @chsize that
> + * has the same matching status as the first chunk.
>   */
>  static int compare_buffers(const uint8_t *buf1, const uint8_t *buf2,
> -   int64_t bytes, int64_t *pnum)
> +   int64_t bytes, uint64_t chsize, int64_t *pnum)

uint64_t might be too large, depending on the platform. memcmp() takes
size_t, so in theory we can get truncation.

I assume that we won't see a problem in practice because you'll never
pass that large values, but I'd prefer making chsize size_t here.

Kevin

[PATCH v3 4/8] qemu-img: add chunk size parameter to compare_buffers()

[Andrey Drobyshev via]

Add @chsize param to the function which, if non-zero, would represent
the chunk size to be used for comparison.  If it's zero, then
BDRV_SECTOR_SIZE is used as default chunk size, which is the previous
behaviour.

In particular, we're going to use this param in img_rebase() to make the
write requests aligned to a predefined alignment value.

Signed-off-by: Andrey Drobyshev 
Reviewed-by: Eric Blake 
Reviewed-by: Hanna Czenczek 
---
 qemu-img.c | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 4dc91505bf..0f67b021f7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1274,23 +1274,29 @@ static int is_allocated_sectors_min(const uint8_t *buf, 
int n, int *pnum,
 }
 
 /*
- * Compares two buffers sector by sector. Returns 0 if the first
- * sector of each buffer matches, non-zero otherwise.
+ * Compares two buffers chunk by chunk, where @chsize is the chunk size.
+ * If @chsize is 0, default chunk size of BDRV_SECTOR_SIZE is used.
+ * Returns 0 if the first chunk of each buffer matches, non-zero otherwise.
  *
- * pnum is set to the sector-aligned size of the buffer prefix that
- * has the same matching status as the first sector.
+ * @pnum is set to the size of the buffer prefix aligned to @chsize that
+ * has the same matching status as the first chunk.
  */
 static int compare_buffers(const uint8_t *buf1, const uint8_t *buf2,
-   int64_t bytes, int64_t *pnum)
+   int64_t bytes, uint64_t chsize, int64_t *pnum)
 {
 bool res;
-int64_t i = MIN(bytes, BDRV_SECTOR_SIZE);
+int64_t i;
 
 assert(bytes > 0);
 
+if (!chsize) {
+chsize = BDRV_SECTOR_SIZE;
+}
+i = MIN(bytes, chsize);
+
 res = !!memcmp(buf1, buf2, i);
 while (i < bytes) {
-int64_t len = MIN(bytes - i, BDRV_SECTOR_SIZE);
+int64_t len = MIN(bytes - i, chsize);
 
 if (!!memcmp(buf1 + i, buf2 + i, len) != res) {
 break;
@@ -1559,7 +1565,7 @@ static int img_compare(int argc, char **argv)
 ret = 4;
 goto out;
 }
-ret = compare_buffers(buf1, buf2, chunk, &pnum);
+ret = compare_buffers(buf1, buf2, chunk, 0, &pnum);
 if (ret || pnum != chunk) {
 qprintf(quiet, "Content mismatch at offset %" PRId64 "!\n",
 offset + (ret ? 0 : pnum));
@@ -3878,7 +3884,7 @@ static int img_rebase(int argc, char **argv)
 int64_t pnum;
 
 if (compare_buffers(buf_old + written, buf_new + written,
-n - written, &pnum))
+n - written, 0, &pnum))
 {
 if (buf_old_is_zero) {
 ret = blk_pwrite_zeroes(blk, offset + written, pnum, 
0);
-- 
2.39.3