From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58109) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SiA0Z-0006nF-2q for qemu-devel@nongnu.org; Fri, 22 Jun 2012 15:59:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SiA0X-0004rh-3e for qemu-devel@nongnu.org; Fri, 22 Jun 2012 15:59:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6672) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SiA0W-0004rX-Rf for qemu-devel@nongnu.org; Fri, 22 Jun 2012 15:59:05 -0400 Message-ID: <4FE4CE7E.6030701@redhat.com> Date: Fri, 22 Jun 2012 13:58:54 -0600 From: Eric Blake MIME-Version: 1.0 References: <1340390174-7493-1-git-send-email-coreyb@linux.vnet.ibm.com> <1340390174-7493-8-git-send-email-coreyb@linux.vnet.ibm.com> In-Reply-To: <1340390174-7493-8-git-send-email-coreyb@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------enig7F844E5A4AEF502C1F65604C" Subject: Re: [Qemu-devel] [PATCH v4 7/7] osdep: Enable qemu_open to dup pre-opened fd List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Bryant Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig7F844E5A4AEF502C1F65604C Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 06/22/2012 12:36 PM, Corey Bryant wrote: > This patch adds support to qemu_open to dup(fd) a pre-opened file > descriptor if the filename is of the format /dev/fd/X. >=20 > This can be used when QEMU is restricted from opening files, and > the management application opens files on QEMU's behalf. >=20 > If the fd was passed to the monitor with the pass-fd command, it > must be explicitly closed with the 'closefd' command when it is > no longer required, in order to prevent fd leaks. >=20 > +static int qemu_dup(int fd, int flags) > +{ > + int ret; > + int serrno; > + > + if (flags & O_CLOEXEC) { > + ret =3D fcntl(fd, F_DUPFD_CLOEXEC, 0); F_DUPFD_CLOEXEC is required by POSIX, but not implemented on all platforms yet. Do you need to be checking with #ifdef F_DUPFD_CLOEXEC to avoid compilation failure? > + if (ret =3D=3D -1 && errno =3D=3D EINVAL) { > + ret =3D dup(fd); > + if (ret =3D=3D -1) { > + goto fail; > + } > + if (fcntl_setfl(ret, O_CLOEXEC, (flags & O_CLOEXEC) ? 1 : = 0) < 0) { Broken. O_CLOEXEC _only_ affects open(); to change it on an existing fd, you have to use fcntl(F_GETFD/F_SETFD) (not F_GETFL/F_SETFL). > + > + if ((fcntl_setfl(ret, O_APPEND, (flags & O_APPEND) ? 1 : 0) = < 0) || > + (fcntl_setfl(ret, O_ASYNC, (flags & O_ASYNC) ? 1 : 0) = < 0) || > + (fcntl_setfl(ret, O_DIRECT, (flags & O_DIRECT) ? 1 : 0) = < 0) || > + (fcntl_setfl(ret, O_LARGEFILE, (flags & O_LARGEFILE) ? 1 : 0) = < 0) || Pointless. O_LARGEFILE should _always_ be set, since we are compiling for 64-bit off_t always. > + (fcntl_setfl(ret, O_NDELAY, (flags & O_NDELAY) ? 1 : 0) = < 0) || > + (fcntl_setfl(ret, O_NOATIME, (flags & O_NOATIME) ? 1 : 0) = < 0) || > + (fcntl_setfl(ret, O_NOCTTY, (flags & O_NOCTTY) ? 1 : 0) = < 0) || > + (fcntl_setfl(ret, O_NONBLOCK, (flags & O_NONBLOCK) ? 1 : 0) = < 0) || > + (fcntl_setfl(ret, O_SYNC, (flags & O_SYNC) ? 1 : 0) = < 0)) { Yuck. That's a lot of syscalls (1 per fcntl_setfl() if they are already set correctly, and 2 per fcntl_setfl() call if we are toggling each one). It might be better to combine this into at most 2 fcntl() calls, instead of a long sequence. > + /* Get the existing fd's flags */ > + eflags =3D fcntl(fd, F_GETFL); > + if (eflags =3D=3D -1) { > + return -1; > + } > + > + if (((flags & O_RDWR) !=3D (eflags & O_RDWR)) || > + ((flags & O_RDONLY) !=3D (eflags & O_RDONLY)) || > + ((flags & O_WRONLY) !=3D (eflags & O_WRONLY))) { Broken. O_RDWR, O_RDONLY, and O_WRONLY are NOT bitmasks, but are values in the range of O_ACCMODE. In particular, O_RDONLY=3D=3D0 on some platfo= rms (Linux), and =3D=3D1 on others (Hurd), and although POSIX recommends that= O_RDWR=3D=3D(O_RDONLY|O_WRONLY) for any new systems, no one has really do= ne that except Hurd. A correct way to write this is: switch (flags & O_ACCMODE) { case O_RDWR: if ((eflags & O_ACCMODE) !=3D O_RDWR) { goto error; break; case O_RDONLY: if ((eflags & O_ACCMODE) !=3D O_RDONLY) { goto error; break; case O_RDONLY: if ((eflags & O_ACCMODE) !=3D O_RDONLY) { goto error; break; default: goto error: } [Technically, POSIX also requires O_ACCMODE to include O_SEARCH and O_EXEC, although those two constants might be the same value; but right now Linux has not yet implemented that bit; but unless qemu ever gains the need to open executable binaries with O_EXEC or directories with O_SEARCH, we probably don't have to worry about that aspect of O_ACCMODE here.] > + errno =3D EACCES; > + return -1; > + } > + > + if (fcntl_setfl(fd, O_CLOEXEC, 1) < 0) { Again, broken. Besides, why are you attempting it both here and in qemu_dup()? Shouldn't once be enough? > + return -1; > + } > + > + return qemu_dup(fd, flags); --=20 Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --------------enig7F844E5A4AEF502C1F65604C 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://enigmail.mozdev.org/ iQEcBAEBCAAGBQJP5M5+AAoJEKeha0olJ0Nq7TgH/Agq2M8gYs3OhI9O4M/Z8KsO xwgr03y4n+21A0r40gjltgVLxJuk76gsKBG8MH0K7qHoKV0D5NVbNv4rTbxrCRVn ygc2cpjI2z4j6HIi1GWb1hakvRH3r0UPiPE3yjOOQQZC/IXBnXej1CLRClSw3BYg B/8Fn6e3ZkTyKrmt9UW8e4BYpeDFr4djLqWmInHRDg2CgJW8pbw/TlTrjv3ojYl+ 5daU25GtmYtbpgHa+eF/k317ptvjqofExmetCM+pqwAQ+luDqt1v8WmV+ir4Ex7v AjSjwqLdJzNDoQJw4bKRTT0KszYRCaec/qu8MEr2g8MhSwPcwqmZ3B/1e/OE7oc= =iz08 -----END PGP SIGNATURE----- --------------enig7F844E5A4AEF502C1F65604C--