From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56953) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UI8kQ-0005Fm-AQ for qemu-devel@nongnu.org; Tue, 19 Mar 2013 22:27:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UI8kO-0000ZV-So for qemu-devel@nongnu.org; Tue, 19 Mar 2013 22:27:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15610) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UI8kO-0000Yz-KR for qemu-devel@nongnu.org; Tue, 19 Mar 2013 22:27:24 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2K2RN77002765 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 19 Mar 2013 22:27:23 -0400 Message-ID: <51491E8A.5010305@redhat.com> Date: Tue, 19 Mar 2013 20:27:22 -0600 From: Eric Blake MIME-Version: 1.0 References: <1363627441-8297-1-git-send-email-kwolf@redhat.com> <1363627441-8297-11-git-send-email-kwolf@redhat.com> In-Reply-To: <1363627441-8297-11-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2TPVLHIOHJXKRRXKODOLV" Subject: Re: [Qemu-devel] [PATCH 10/11] block: Allow omitting the file name when using driver-specific options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: pbonzini@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2TPVLHIOHJXKRRXKODOLV Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 03/18/2013 11:24 AM, Kevin Wolf wrote: > After this patch, using -drive with an empty file name continues to ope= n > the file if driver-specific options are used. If no driver-specific > options are specified, the semantics stay as it was: It defines a drive= > without an inserted medium. >=20 > In order to achieve this, bdrv_open() must be made safe to work with a > NULL filename parameter. The assumption that is made is that only block= > drivers which implement bdrv_parse_filename() support using driver > specific options and could therefore work without a filename. These > drivers must make sure to cope with NULL in their implementation of > .bdrv_open() (this is only NBD for now). For all other drivers, the > block layer code will make sure to error out before calling into their > code - they can't possibly work without a filename. >=20 > Now an NBD connection can be opened like this: >=20 > qemu-system-x86_64 -drive file.driver=3Dnbd,file.port=3D1234,file.hos= t=3D::1 Slick. >=20 > Signed-off-by: Kevin Wolf > --- > block.c | 49 +++++++++++++++++++++++++++++++++++++++= -------- > block/nbd.c | 1 - > blockdev.c | 10 +++++++--- > include/block/block_int.h | 3 +++ > 4 files changed, 51 insertions(+), 12 deletions(-) > +++ b/block/nbd.c > @@ -174,7 +174,6 @@ static void nbd_parse_filename(const char *filename= , QDict *options, > =20 > /* extract the host_spec - fail if it's not nbd:... */ > if (!strstart(file, "nbd:", &host_spec)) { > - error_setg(errp, "File name string for NBD must start with 'nb= d:'"); > goto out; > } Is this really right? The code allows me to pass both file=3D and file.driver=3D at once; so what if I pass -drive file.driver=3Dnbd,file=3D= xyz. Prior to this patch, I'd get a nice message about file=3Dxyz not startin= g with nbd:, but now there is a silent failure. I think it might be better if you keep the error_setg(), and instead rewrite the if on the previous line: if (file && !strstart(file, "nbd:", &host_spec)) { Also, since it looks like the code allows me to pass both file.driver=3D and file=3D at once, if I pass both pieces of information, is there any sanity checking that the two are identical, and/or should we error out and declare that if driver options are used then nbd requires a NULL filename? > @@ -697,10 +701,10 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockIn= terfaceType block_default_type) > if (ret < 0) { > if (ret =3D=3D -EMEDIUMTYPE) { > error_report("could not open disk image %s: not in %s form= at", > - file, drv->format_name); > + file ?: dinfo->id, drv->format_name); You're not the first person to use this gcc extension, but the more instances we add, the harder it will be to scrub them back out if someone ever insists on portability to another compiler. > } else { > error_report("could not open disk image %s: %s", > - file, strerror(-ret)); > + file ?: dinfo->id, strerror(-ret)); > } > goto err; > } > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 1b06a11..0986a2d 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -75,6 +75,9 @@ struct BlockDriver { > int instance_size; > int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *fi= lename); > int (*bdrv_probe_device)(const char *filename); > + > + /* Any driver implementing this callback is expected to be able to= handle > + * NULL file names in its .bdrv_open() implementation */ > void (*bdrv_parse_filename)(const char *filename, QDict *options, = Error **errp); > =20 > /* For handling image reopen for split or non-split files */ >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2TPVLHIOHJXKRRXKODOLV 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/ iQEcBAEBCAAGBQJRSR6KAAoJEKeha0olJ0NqxcEH/RkTbfXyo5k8DJHM0t9ASgqI oIdwnDhoTEfcehkRmWGZ/WEZRljvIGcIxwqEE0Q49oqe6dhglf/Otpyafa2fnXxa yMsLVyWVE/nh8q2VnVbBntE6Q6c++HcpeRPSWZimhu1lIhxb7hKV/LPPFDUxMK5+ i9jqCOM7PPoKd1UDlVz7earlGlzt0oF+H7WRK1oioQ1p2z9mAOu9RVuAdoKjeh9g gC5VrBAiCrwuJZhhlielv35G/HlY1qexbhP/USWI4X4gMu8uor5R3TqRC/gGNuUi LQp+eboBhFcpQCIi4SIBS6ASeFOwSRwaRw7nIBhgEIohqdZnqJLFePMNFa0lz9A= =O6ao -----END PGP SIGNATURE----- ------enig2TPVLHIOHJXKRRXKODOLV--