From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34396) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URpXH-00062j-Iu for qemu-devel@nongnu.org; Mon, 15 Apr 2013 15:57:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1URpXG-0005F8-C5 for qemu-devel@nongnu.org; Mon, 15 Apr 2013 15:57:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9791) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1URpXG-0005F0-4E for qemu-devel@nongnu.org; Mon, 15 Apr 2013 15:57:54 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3FJvra3015963 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 15 Apr 2013 15:57:53 -0400 Message-ID: <516C5BC0.5030803@redhat.com> Date: Mon, 15 Apr 2013 13:57:52 -0600 From: Eric Blake MIME-Version: 1.0 References: <1365799688-19918-1-git-send-email-kwolf@redhat.com> <1365799688-19918-9-git-send-email-kwolf@redhat.com> In-Reply-To: <1365799688-19918-9-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2BONMKCGFVDPEGPUBWDKD" Subject: Re: [Qemu-devel] [PATCH 08/15] curl: Use bdrv_open options instead of filename List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2BONMKCGFVDPEGPUBWDKD Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/12/2013 02:48 PM, Kevin Wolf wrote: > Signed-off-by: Kevin Wolf > --- > block/curl.c | 153 +++++++++++++++++++++++++++++++++++++++------------= -------- > 1 file changed, 102 insertions(+), 51 deletions(-) >=20 > @@ -369,29 +364,78 @@ static int curl_open(BlockDriverState *bs, const = char *filename, > ra_val =3D ra + 1; > ra -=3D strlen(RA_OPTSTR) - 1; > *ra =3D '\0'; > - s->readahead_size =3D atoi(ra_val); Good riddance - atoi() is evil because it can't detect overflow. > - break; > - } else { > - break; > + qdict_put(options, "readahead", qstring_from_str(r= a_val)); So now we just pass the string on to the option parser,... > +static QemuOptsList runtime_opts =3D { > + .name =3D "curl", > + .head =3D QTAILQ_HEAD_INITIALIZER(runtime_opts.head), > + .desc =3D { > + { > + .name =3D "url", > + .type =3D QEMU_OPT_STRING, > + .help =3D "URL to open", > + }, > + { > + .name =3D "readahead", > + .type =3D QEMU_OPT_SIZE, > + .help =3D "Readahead size", =2E..and this new option lets QEMU_OPT_SIZE detect crazy strings that wer= e previously treated as '0' by the old atoi(). You should probably mention this bug fix in the commit message as being intentional. > @@ -568,63 +614,68 @@ static int64_t curl_getlength(BlockDriverState *b= s) > } > =20 > static BlockDriver bdrv_http =3D { > - .format_name =3D "http", > - .protocol_name =3D "http", > + .format_name =3D "http", > + .protocol_name =3D "http", > =20 > - .instance_size =3D sizeof(BDRVCURLState), > - .bdrv_file_open =3D curl_open, > - .bdrv_close =3D curl_close, > - .bdrv_getlength =3D curl_getlength, > + .instance_size =3D sizeof(BDRVCURLState), > + .bdrv_parse_filename =3D curl_parse_filename, > + .bdrv_file_open =3D curl_open, > + .bdrv_close =3D curl_close, > + .bdrv_getlength =3D curl_getlength, > =20 > - .bdrv_aio_readv =3D curl_aio_readv, > + .bdrv_aio_readv =3D curl_aio_readv, On this patch, you reindented ALL callbacks to the same width. But in 6/15 and 7/15, you reindented only the callbacks in the =2Ebdrv_parse_filename section. You should probably be consistent betwee= n patches (I actually prefer indenting the entire BlockDriver initialization consistently, as done in this patch, but didn't ding patch 6 or 7 at the time because they looked innocuous enough in isolatio= n). Since I only called out whitespace issues (and even then, only for commits other than this) and a weakness in the commit message, the code itself deserves: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2BONMKCGFVDPEGPUBWDKD 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.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJRbFvAAAoJEKeha0olJ0NqZeYIAKaga95JRFuvnHXflzCiug4v /fuiPBIbCOgMK/gp14fI4hKmJa2HkjBTNgYjsG2Vq30NkJ2LbksoiaKJhktOJJ8a NYZrLN1OyZNTy6Y2kuXLD2WfjgPy3Kpi48Qg4lZRGmu5MSl8Lvq5L90gb5HwK33A Pec5uzJ2XSqxQgex60LDvX7T8FdBXtaE5O9kFNsWETXIsOOsld6z6NPlPpul+nYm GWkSeTKMyJK6ZPTh8lHWpaqyorqmit8UlsxuAWCwhFcQ8orSJeCyvki9LRwpNq+a /FPlM5kM9By6hqFHiUqe0imOuXd2Fj9uaY/e2ch06YMWDc9GsFvzQYYJ6BY/uwk= =z8bO -----END PGP SIGNATURE----- ------enig2BONMKCGFVDPEGPUBWDKD--