From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36481) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJjh7-0005DA-40 for qemu-devel@nongnu.org; Fri, 06 Feb 2015 09:15:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJjh3-0006DD-PH for qemu-devel@nongnu.org; Fri, 06 Feb 2015 09:15:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35858) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJjh3-0006D4-Fx for qemu-devel@nongnu.org; Fri, 06 Feb 2015 09:15:37 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t16EFaEC007608 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 6 Feb 2015 09:15:37 -0500 Message-ID: <54D4CC86.9020404@redhat.com> Date: Fri, 06 Feb 2015 09:15:34 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423151912-24863-1-git-send-email-mreitz@redhat.com> <20150206140858.GE13081@noname.redhat.com> In-Reply-To: <20150206140858.GE13081@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] qcow2: Rewrite qcow2_alloc_bytes() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi On 2015-02-06 at 09:08, Kevin Wolf wrote: > Am 05.02.2015 um 16:58 hat Max Reitz geschrieben: >> qcow2_alloc_bytes() is a function with insufficient error handling and >> an unnecessary goto. This patch rewrites it. >> >> Signed-off-by: Max Reitz >> --- >> v2: >> - s/free_cluster_index/free_byte_index/ [Eric] >> - added an assertion at the start of the function that >> s->free_byte_offset is either 0 or points to the tail of a cluster >> (but never to the start) >> - use ROUND_UP() instead of start_of_cluster() + cluster_size [Eric] >> - added an assertion that s->free_byte_offset is set before using it >> [Eric] >> --- >> block/qcow2-refcount.c | 77 +++++++++++++++++++++++++++++--------------------- >> 1 file changed, 45 insertions(+), 32 deletions(-) >> [snip] > The patch looks correct to me. Let me know if you'd like to address the > point I made above, or if I should apply it as it is. Good question. On one hand, I like your suggestion because it would indeed make the code shorter. On the other hand, I somehow feel better using functions that are prefixed qcow2_* because I feel like it might make the code harder to read if sometimes we use qcow2_alloc_clusters() and alloc_clusters_noref() at other times; and sometimes we use qcow2_update_cluster_refcount() and update_refcount() at other times. But then again, it might make sense to have this function mirror qcow2_alloc_clusters(), which does use alloc_clusters_noref() and update_refcount() (of course). So I think I'll go with your suggestion, thanks! Max