From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XuJhN-00052u-HI for qemu-devel@nongnu.org; Fri, 28 Nov 2014 06:27:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XuJhE-0007V6-Bv for qemu-devel@nongnu.org; Fri, 28 Nov 2014 06:26:53 -0500 Received: from mail-wg0-x22d.google.com ([2a00:1450:400c:c00::22d]:46897) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XuJhE-0007V0-1A for qemu-devel@nongnu.org; Fri, 28 Nov 2014 06:26:44 -0500 Received: by mail-wg0-f45.google.com with SMTP id b13so8607592wgh.32 for ; Fri, 28 Nov 2014 03:26:43 -0800 (PST) Date: Fri, 28 Nov 2014 11:26:41 +0000 From: Stefan Hajnoczi Message-ID: <20141128112641.GD13631@stefanha-thinkpad.redhat.com> 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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="iVCmgExH7+hIHJ1A" Content-Disposition: inline In-Reply-To: <54774424.80200@redhat.com> 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: Max Reitz Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi --iVCmgExH7+hIHJ1A Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =3D cluster_index & (s->refcount_block_size - 1); > >>- refcount =3D be16_to_cpu(refcount_block[block_index]); > >>+ refcount =3D s->get_refcount(refcount_block, block_index); > >>+ if (refcount < 0) { > >>+ ret =3D -ERANGE; > >>+ goto fail; > >>+ } > >Here again. > > > >>@@ -1206,11 +1236,14 @@ static int inc_refcounts(BlockDriverState *bs, > >> } > >> } > >>- if (++(*refcount_table)[k] =3D=3D 0) { > >>+ refcount =3D 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. >=20 > 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 li= ne > which you omitted directly after this clips the refcount values against > s->refcount_max which is INT64_MAX for 64-bit refcounts. >=20 > Therefore, no overflows possible in the refcount checking functions, beca= use > the refcount checking functions don't introduce the overflows themselves. > The other overflow checks are only in place to reject faulty images provi= ded > from the outside. I see, that makes sense. > >>@@ -1726,7 +1761,7 @@ static void compare_refcounts(BlockDriverState *b= s, BdrvCheckResult *res, > >> continue; > >> } > >>- refcount2 =3D refcount_table[i]; > >>+ refcount2 =3D 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? >=20 > 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. >=20 > >>+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. >=20 > Actually, it does. It's just that the implementation provided here does n= ot > support it. >=20 > 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. --iVCmgExH7+hIHJ1A Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUeFvxAAoJEJykq7OBq3PIvogIAKrBpS4RjIPyYgXIs+3OwEWv qndRhsWZOuioTmlEyNm02xvtdJTyzmUC3uNGSe8WmZKgg6tPJ38/GPO2W20GLmPH 3ugtafhIoodOALaPdo+B3y/4WCOGog7CZvQEAGfZ2qAa6UD8JRyDyTuT+YnYf8Vn pFy5AOXjXVXPRUdEq7dPGNcUtWfAgz+bonAMFj/dlj5EdmBNg6ojDgQujWhg6ToD dSm2aj89qvHboLH8rGgzWW7rfQ3TMR2Ml5otzToeCgVm5IYi+8j5qsJ9L7NvTfvB /+YnDiyvCa3ddIpBBtex1TC26CnrqOjIyUU4xjeXN2UEPIqnlZrPa6ZSvd7Hp7g= =O4ar -----END PGP SIGNATURE----- --iVCmgExH7+hIHJ1A--