[{"id":1776659,"web_url":"http://patchwork.ozlabs.org/comment/1776659/","msgid":"<0dc1b91f-c996-9b50-731c-54663f077a6d@redhat.com>","list_archive_url":null,"date":"2017-09-27T22:25:59","subject":"Re: [Qemu-devel] [PATCH v4 18/23] qemu-img: Change\n\tcompare_sectors() to be byte-based","submitter":{"id":64343,"url":"http://patchwork.ozlabs.org/api/people/64343/","name":"John Snow","email":"jsnow@redhat.com"},"content":"On 09/13/2017 12:03 PM, Eric Blake wrote:\n> In the continuing quest to make more things byte-based, change\n> compare_sectors(), renaming it to compare_buffers() in the\n> process. Note that one caller (qemu-img compare) only cares\n> about the first difference, while the other (qemu-img rebase)\n> cares about how many consecutive sectors have the same\n> equal/different status; however, this patch does not bother to\n> micro-optimize the compare case to avoid the comparisons of\n> sectors beyond the first mismatch. Both callers are always\n> passing valid buffers in, so the initial check for buffer size\n> can be turned into an assertion.\n> \n> Signed-off-by: Eric Blake \n> \n> ---\n> v3: new patch\n> ---\n> qemu-img.c | 55 +++++++++++++++++++++++++++----------------------------\n> 1 file changed, 27 insertions(+), 28 deletions(-)\n> \n> diff --git a/qemu-img.c b/qemu-img.c\n> index 2e05f92e85..034122eba5 100644\n> --- a/qemu-img.c\n> +++ b/qemu-img.c\n> @@ -1155,31 +1155,28 @@ static int is_allocated_sectors_min(const uint8_t *buf, int n, int *pnum,\n> }\n> \n> /*\n> - * Compares two buffers sector by sector. Returns 0 if the first sector of both\n> - * buffers matches, non-zero otherwise.\n> + * Compares two buffers sector by sector. Returns 0 if the first\n> + * sector of each buffer matches, non-zero otherwise.\n> *\n> - * pnum is set to the number of sectors (including and immediately following\n> - * the first one) that are known to have the same comparison result\n> + * pnum is set to the sector-aligned size of the buffer prefix that\n> + * has the same matching status as the first sector.\n> */\n> -static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n,\n> - int *pnum)\n> +static int compare_buffers(const uint8_t *buf1, const uint8_t *buf2,\n> + int64_t bytes, int64_t *pnum)\n> {\n> bool res;\n> - int i;\n> + int64_t i = MIN(bytes, BDRV_SECTOR_SIZE);\n> \n> - if (n <= 0) {\n> - *pnum = 0;\n> - return 0;\n> - }\n> + assert(bytes > 0);\n> \n> - res = !!memcmp(buf1, buf2, 512);\n> - for(i = 1; i < n; i++) {\n> - buf1 += 512;\n> - buf2 += 512;\n> + res = !!memcmp(buf1, buf2, i);\n\nIt is temporarily confusing that 'i' is never again used for this\nparticular parameter, because\n\n> + while (i < bytes) {\n\nThis gives the brief impression that we might be looping in a way that\nchanges the comparison size passed to memcmp, which isn't true.\n\nJust me being cranky, though. It's probably still the best way, because\nof how you have to prime the loop. Doing it the literal-minded way\nrequires an extra i += len, so:\n\nReviewed-by: John Snow ","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx09.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jsnow@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y2XVR2jqQz9t6C\n\tfor ;\n\tThu, 28 Sep 2017 08:26:39 +1000 (AEST)","from localhost ([::1]:56568 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dxKmr-0001Gd-HS\n\tfor incoming@patchwork.ozlabs.org; Wed, 27 Sep 2017 18:26:37 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:58310)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dxKmT-0001F5-Ce\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 18:26:14 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dxKmS-0000gD-Dp\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 18:26:13 -0400","from mx1.redhat.com ([209.132.183.28]:48718)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dxKmL-0000eJ-W5; Wed, 27 Sep 2017 18:26:06 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 12FDA3566E6;\n\tWed, 27 Sep 2017 22:26:05 +0000 (UTC)","from [10.10.121.69] (ovpn-121-69.rdu2.redhat.com [10.10.121.69])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 3FB6A8D547;\n\tWed, 27 Sep 2017 22:26:00 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 12FDA3566E6","To":"Eric Blake , qemu-devel@nongnu.org","References":"<20170913160333.23622-1-eblake@redhat.com>\n\t<20170913160333.23622-19-eblake@redhat.com>","From":"John Snow ","Message-ID":"<0dc1b91f-c996-9b50-731c-54663f077a6d@redhat.com>","Date":"Wed, 27 Sep 2017 18:25:59 -0400","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170913160333.23622-19-eblake@redhat.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.38]);\n\tWed, 27 Sep 2017 22:26:05 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v4 18/23] qemu-img: Change\n\tcompare_sectors() to be byte-based","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, famz@redhat.com, qemu-block@nongnu.org,\n\tMax Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]