From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42298) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TM4o7-00006I-9B for qemu-devel@nongnu.org; Wed, 10 Oct 2012 18:31:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TM4o6-0006g4-0N for qemu-devel@nongnu.org; Wed, 10 Oct 2012 18:31:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57977) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TM4o5-0006g0-Nj for qemu-devel@nongnu.org; Wed, 10 Oct 2012 18:31:13 -0400 Message-ID: <5075F727.1060608@redhat.com> Date: Wed, 10 Oct 2012 16:31:03 -0600 From: Eric Blake MIME-Version: 1.0 References: <1349878805-16352-1-git-send-email-coreyb@linux.vnet.ibm.com> <1349878805-16352-4-git-send-email-coreyb@linux.vnet.ibm.com> In-Reply-To: <1349878805-16352-4-git-send-email-coreyb@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig8623E77044211D7717952CE8" Subject: Re: [Qemu-devel] [PATCH v2 3/3] qemu-config: Add new -add-fd command line option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Bryant Cc: kwolf@redhat.com, libvir-list@redhat.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig8623E77044211D7717952CE8 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 10/10/2012 08:20 AM, Corey Bryant wrote: > This option can be used for passing file descriptors on the > command line. It mirrors the existing add-fd QMP command which > allows an fd to be passed to QEMU via SCM_RIGHTS and added to an > fd set. >=20 > This can be combined with commands such as -drive to link file > descriptors in an fd set to a drive: >=20 > qemu-kvm -add-fd fd=3D4,set=3D2,opaque=3D"rdwr:/path/to/file" > -add-fd fd=3D5,set=3D2,opaque=3D"rdonly:/path/to/file" > -drive file=3D/dev/fdset/2,index=3D0,media=3Ddisk >=20 > This example adds fds 4 and 5, and the accompanying opaque > strings to the fd set with ID=3D2. qemu_open() already knows how > to handle a filename of this format. qemu_open() searches the > corresponding fd set for an fd and when it finds a match, QEMU > goes on to use a dup of that fd just like it would have used an > fd that it opened itself. Missing some argument validation. Earlier, I complained about set validation, now I'm going to complain about fd validation. > +static int parse_add_fd(QemuOpts *opts, void *opaque) > +{ > + int fd; > + int64_t fdset_id; > + const char *fd_opaque =3D NULL; > + > + fd =3D qemu_opt_get_number(opts, "fd", -1); > + fdset_id =3D qemu_opt_get_number(opts, "set", -1); > + fd_opaque =3D qemu_opt_get(opts, "opaque"); If I call 'qemu -add-fd fd=3D-2,set=3D1', that had better fail because fd= -2 does not exist. Likewise if I call 'qemu -add-fd fd=3D10000000,set=3D1' (here, picking an fd that I know can't be opened). More subtly, 'qemu -add-fd fd=3D4,set=3D1 4<&-' should fail, since fd 4 was not inherited do= wn to the qemu process. Fuzzier is whether 'qemu -add-fd fd=3D0,set=3D1' ma= kes sense, as that would then make stdin be competing for multiple uses; this would be a situation that the monitor command can't duplicate, so you have introduced new ways to possibly break things from the command line. I'm thinking it's safest to reject fd of 0, 1, or 2 for now, and only relax it later IF we can prove that it would be both useful and safe= =2E So it sounds like you need something like fcntl(fd,F_GETFL) to see that an the fd was actually inherited, as well as validating that fd > STDERR_FILENO. Another missing validation check is for duplicate use. With the monitor command, you ALWAYS have a unique fd (thanks to SCM_RIGHTS). But with the command line, I can type 'qemu -add-fd fd=3D4,set=3D1 -add-fd fd=3D4,set=3D2'. Oops - I've now corrupted your set layout, unless you validate that every fd requested in -add-fd does not already reside in any existing set. Thinking aloud: On the other hand, being able to pass in one fd to multiple sets MIGHT be useful; in the SCM_RIGHTS monitor command case, I can pass the same fd from the management perspective into multiple sets, even though in qemu's perspective, there will be multiple fds created (one per call). Perhaps instead of directly adding the inherited fd to a set, and having to then sweep all sets to check for duplicates, it might make sense to add dup(fd) to a set, so that if I call: qemu -add-fd fd=3D4,set=3D1 -add-fd fd=3D4,set=3D2 -add-fd fd=3D5,set=3D2= what REALLY happens is that qemu adds dup(4)=3D=3D6 to set 1, dup(4)=3D=3D= 7 to set 2, and dup(5)=3D=3D8 to set 3. Then, after all ALL -add-fd have been= processed, qemu then does another pass through them calling close(4) and close(5) (to avoid holding the original fds open indefinitely if the corresponding sets are discarded). Hmm, notice that if you dup() before adding to a set, then that the dup() resolves my first complaint of validating that the inherited fd exists; it also changes the problem of dealing with fd 0 (it would now be valid to add fd 0 to any number of sets; but a final cleanup loop had better be careful to not call close(0) unintentionally). Personally, though, I'm not sure the complexity of using dup() buys us anything, so I'm happy with the simpler solution of using fd as-is, coupled with a check for no reuse of an fd already in a set. --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig8623E77044211D7717952CE8 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.12 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQEcBAEBCAAGBQJQdfcnAAoJEKeha0olJ0Nq5ZAH/2GXMvSveQf4y2SJLYL7SsvB ZYpc3jNuC8yIUk9HCGl623yJqbPV5e/r6T3gSQg4Vxc1I4S2NUKWOiHJKffmcqjx zkAdu16J5rQmbVZI6hd0eIqN/ln6/Dozat+3o1CNk5wq44gMGoncnrqXAAgINuEf oPupCTfuOm87ciP4Cns5nfSgt29ewiTxgAR8bSMUgDU3OtREdQt5Y7e/PWOTl6PM WA5Ej4SPtjVLsac8K9rxTJmbn6zXmNB6uKKbQymoq4n9PI9kwUxqE9+ygb4NrNT6 +AZCjdFynsUWNKE3z1IdWa7xYF8UDSGVb0jzFjS0aov+99qHcUcv93FjESn5zfw= =Syoc -----END PGP SIGNATURE----- --------------enig8623E77044211D7717952CE8--