From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57086) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRKkd-0008Eu-D1 for qemu-devel@nongnu.org; Tue, 09 Sep 2014 08:42:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XRKkX-0007WV-9l for qemu-devel@nongnu.org; Tue, 09 Sep 2014 08:42:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:17920) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRKkX-0007WI-2O for qemu-devel@nongnu.org; Tue, 09 Sep 2014 08:42:21 -0400 Message-ID: <540EF5A5.3030909@redhat.com> Date: Tue, 09 Sep 2014 06:42:13 -0600 From: Eric Blake MIME-Version: 1.0 References: In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mwrpsQxeulp88cOFh2vgPhgxCDse6eNRV" Subject: Re: [Qemu-devel] [PATCH v14 3/5] qapi: introduce PreallocMode and new PreallocModes full and falloc. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hu Tao , qemu-devel@nongnu.org Cc: Kevin Wolf , Fam Zheng , "Richard W.M. Jones" , Max Reitz , Stefan Hajnoczi , Yasunori Goto This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --mwrpsQxeulp88cOFh2vgPhgxCDse6eNRV Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 09/08/2014 09:54 PM, Hu Tao wrote: > This patch prepares for the subsequent patches. >=20 > Signed-off-by: Hu Tao > Reviewed-by: Max Reitz > Reviewed-by: Kevin Wolf > --- > block/qcow2.c | 23 +++++++++++++++-------- > qapi/block-core.json | 17 +++++++++++++++++ > tests/qemu-iotests/049.out | 2 +- > 3 files changed, 33 insertions(+), 9 deletions(-) >=20 > buf =3D qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC); > - if (!buf || !strcmp(buf, "off")) { > - prealloc =3D 0; > - } else if (!strcmp(buf, "metadata")) { > - prealloc =3D 1; > - } else { > - error_setg(errp, "Invalid preallocation mode: '%s'", buf); > + prealloc =3D qapi_enum_parse(PreallocMode_lookup, buf, > + PREALLOC_MODE_MAX, PREALLOC_MODE_OFF, > + &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > ret =3D -EINVAL; > goto finish; > } > @@ -1958,6 +1958,13 @@ static int qcow2_create(const char *filename, Qe= muOpts *opts, Error **errp) > flags |=3D BLOCK_FLAG_LAZY_REFCOUNTS; > } > =20 > + if (prealloc && prealloc !=3D PREALLOC_MODE_METADATA) { > + ret =3D -EINVAL; > + error_setg(errp, "Unsupported preallocate mode: %s", > + PreallocMode_lookup[prealloc]); > + goto finish; > + } I _still_ think this looks weird, and would be better as either: if (prealloc !=3D PREALLOC_MODE_NONE && prealloc !=3D PREALLOC_MODE_METADATA) { to make it obvious that you are filtering for two acceptable modes, or as= : if (prealloc =3D=3D PREALLOC_MODE_FALLOC || prealloc =3D=3D PREALLOC_MODE_FULL) { to make it obvious the modes that you do not support. But my complaint is not strong enough to prevent this patch, especially if later in the series revisits this code. Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --mwrpsQxeulp88cOFh2vgPhgxCDse6eNRV Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJUDvWlAAoJEKeha0olJ0Nq3AIH/1IflLv76N1fOzkm95I2trQT CKitd+7W9FFoRYmCdbrklPEcUkLMeLkO8V1Z1i18bMQXH4aa/EGzW1J9F2RcLsqY cq4YKPxm5jg7oVeDDgQD5UmddqpZ8A6c3un8C3QVkiouyk5ofZ1/tv0GFRXkS401 DqknxbMyVBqLyB/CVmdOiOm7kPfGhsBTklAMho8HBRCPiWJkYa38qUMdcpSwU9VB ipR5tGnmWfKjD1LJhUHRiCpg5K8nwn5WpMMA8EtJtFPZ15YsG3WE30kemyNsG2c5 JbdAelcip7KqQj9zl3YZS3zSPTPiV+QiUWAqOIvHUGBuoCmM2zHv6+4wEh+BUAU= =rPEA -----END PGP SIGNATURE----- --mwrpsQxeulp88cOFh2vgPhgxCDse6eNRV--