From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52091) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XvkAc-0006oT-Kd for qemu-devel@nongnu.org; Tue, 02 Dec 2014 04:55:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XvkAW-00015l-8Z for qemu-devel@nongnu.org; Tue, 02 Dec 2014 04:54:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46533) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XvkAW-00015b-18 for qemu-devel@nongnu.org; Tue, 02 Dec 2014 04:54:52 -0500 Message-ID: <547D8C67.5050804@redhat.com> Date: Tue, 02 Dec 2014 10:54:47 +0100 From: Max Reitz MIME-Version: 1.0 References: <1416503198-17031-1-git-send-email-mreitz@redhat.com> <1416503198-17031-8-git-send-email-mreitz@redhat.com> <20141127152158.GG15586@stefanha-thinkpad.lan> <54774424.80200@redhat.com> <20141128112641.GD13631@stefanha-thinkpad.redhat.com> In-Reply-To: <20141128112641.GD13631@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 07/22] qcow2: Helper function for refcount modification 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 12:26, Stefan Hajnoczi wrote: > On Thu, Nov 27, 2014 at 04:32:52PM +0100, Max Reitz wrote: >> On 2014-11-27 at 16:21, Stefan Hajnoczi wrote: >>> On Thu, Nov 20, 2014 at 06:06:23PM +0100, Max Reitz wrote: >>>> @@ -583,7 +608,12 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, >>>> /* we can update the count and save it */ >>>> block_index = cluster_index & (s->refcount_block_size - 1); >>>> - refcount = be16_to_cpu(refcount_block[block_index]); >>>> + refcount = s->get_refcount(refcount_block, block_index); >>>> + if (refcount < 0) { >>>> + ret = -ERANGE; >>>> + goto fail; >>>> + } >>> Here again. >>> >>>> @@ -1206,11 +1236,14 @@ static int inc_refcounts(BlockDriverState *bs, >>>> } >>>> } >>>> - if (++(*refcount_table)[k] == 0) { >>>> + refcount = s->get_refcount(*refcount_table, k); >>> Here the refcount < 0 check is missing. That's why it would be simpler >>> to eliminate the refcount < 0 case entirely. >> It's not missing. This is part of the refcount check, as are all the >> following ("in other places"). The refcount check builds a refcount array in >> memory all by itself, so it knows for sure there are no overflows. The line >> which you omitted directly after this clips the refcount values against >> s->refcount_max which is INT64_MAX for 64-bit refcounts. >> >> Therefore, no overflows possible in the refcount checking functions, because >> the refcount checking functions don't introduce the overflows themselves. >> The other overflow checks are only in place to reject faulty images provided >> from the outside. > I see, that makes sense. > >>>> @@ -1726,7 +1761,7 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res, >>>> continue; >>>> } >>>> - refcount2 = refcount_table[i]; >>>> + refcount2 = s->get_refcount(refcount_table, i); >>> Missing here too and in other places. >>> >>>> +typedef uint64_t Qcow2GetRefcountFunc(const void *refcount_array, >>>> + uint64_t index); >>> Should the return type be int64_t? >> No. If it was, we'd have to check for errors every time we call it, but it >> cannot return errors (well, if we let it return uint64_t, it might return >> -ERANGE, but that's exactly what I don't want). Therefore, let it return >> uint64_t so we know this function cannot fail. >> >>>> +typedef void Qcow2SetRefcountFunc(void *refcount_array, >>>> + uint64_t index, uint64_t value); >>> Should value's type be int64_t? Just because the type is unsigned >>> doesn't make (uint64_t)-1ULL a valid value. >> Actually, it does. It's just that the implementation provided here does not >> support it. >> >> Since there is an assertion against that case in the 64-bit implementation >> for this function, I don't have a problem with using int64_t here, though. >> But that would break symmetry with Qcow2GetRefcountFunc(), and I do have a >> reason there not to return a signed value, as explained above. > Okay, so uint64_t is really a different type - it's the qcow2 spec > 64-bit refcount. int64_t is the QEMU implementation's internal > representation. > > This seems error-prone to me. Maybe comments would have helped, but > it's best to eliminate the problem entirely. Why not bite the bullet > and fix up qcow2? > > There are two external function prototypes that need to be changed: > > int64_t qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index); > > int64_t qcow2_update_cluster_refcount(BlockDriverState *bs, > int64_t cluster_index, int addend, > enum qcow2_discard_type type); > > /* Returns 0 on success, -errno on failure */ > int qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index, > uint64_t *refcount); > int qcow2_update_cluster_refcount(BlockDriverState *bs, > int64_t cluster_index, int addend, > enum qcow2_discard_type type, > uint64_t *refcount); > > Have I missed a fundamental reason why the implementation's internal > refcount type cannot be changed from int64_t to uint64_t? > > It would keep the code complexity down and reduce errors. Sounds simple enough. I'll take a look, thanks! Max