From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51619) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xvk8P-0005lj-8D for qemu-devel@nongnu.org; Tue, 02 Dec 2014 04:52:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xvk8I-0000nR-Qz for qemu-devel@nongnu.org; Tue, 02 Dec 2014 04:52:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57243) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xvk8I-0000nM-Iz for qemu-devel@nongnu.org; Tue, 02 Dec 2014 04:52:34 -0500 Message-ID: <547D8BDD.9000108@redhat.com> Date: Tue, 02 Dec 2014 10:52:29 +0100 From: Max Reitz MIME-Version: 1.0 References: <1416503198-17031-1-git-send-email-mreitz@redhat.com> <1416503198-17031-7-git-send-email-mreitz@redhat.com> <20141127150931.GF15586@stefanha-thinkpad.lan> <54773F10.3040102@redhat.com> <20141128104631.GB13631@stefanha-thinkpad.redhat.com> In-Reply-To: <20141128104631.GB13631@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 06/22] qcow2: Helper for refcount array reallocation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 2014-11-28 at 11:46, Stefan Hajnoczi wrote: > On Thu, Nov 27, 2014 at 04:11:12PM +0100, Max Reitz wrote: >> On 2014-11-27 at 16:09, Stefan Hajnoczi wrote: >>> On Thu, Nov 20, 2014 at 06:06:22PM +0100, Max Reitz wrote: >>>> +/** >>>> + * Reallocates *array so that it can hold new_size entries. *size must contain >>>> + * the current number of entries in *array. If the reallocation fails, *array >>>> + * and *size will not be modified and -errno will be returned. If the >>>> + * reallocation is successful, *array will be set to the new buffer and *size >>>> + * will be set to new_size. The size of the reallocated refcount array buffer >>>> + * will be aligned to a cluster boundary, and the newly allocated area will be >>>> + * zeroed. >>>> + */ >>>> +static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array, >>>> + int64_t *size, int64_t new_size) >>>> +{ >>>> + /* Round to clusters so the array can be directly written to disk */ >>>> + size_t old_byte_size = ROUND_UP(refcount_array_byte_size(s, *size), >>>> + s->cluster_size); >>>> + size_t new_byte_size = ROUND_UP(refcount_array_byte_size(s, new_size), >>>> + s->cluster_size); >>>> + uint16_t *new_ptr; >>>> + >>>> + if (new_byte_size <= old_byte_size) { >>>> + *size = new_size; >>>> + return 0; >>>> + } >>> Why not realloc the array to the new smaller size? ... >> Because such a call will actually never happen. I could replace this if () >> by assert(new_byte_size >= old_byte_size); if (new_byte_size == >> old_byte_size), but as I said before, I'm not a friend of assertions when >> the code can deal perfectly well with the "unsupported" case. > It is simpler to put an if statement around the memset. Well, I personally find an assertion simpler; and I will not drop the if (new_byte_size == old_byte_size) because this is a very common case so I don't want to rely on g_realloc() to detect it. Also, Eric proposed it and I'd like to avoid a ping-pong discussion. > Then the function actually frees unused memory Which will never happen, though, because new_byte_size will never be less than old_byte_size. > and readers don't wonder why you decided not to shrink the array. An assertion will clear up things as well. > Less code and slightly better behavior. Well, an assert() is one line, while an if () takes two (if () { and }); the behavior will (hopefully) not change either. But anyway, I'll go with your proposition because, as I said, I don't like assertions if the code can deal perfectly fine with the bad cases. Therefore, if at some point in time we decide to use realloc_refcount_array() to shrink a refcount array, it's better to have planned for that case. Max