Re: [libvirt] [Qemu-devel] [PATCH v4 7/7] osdep: Enable qemu_open to dup pre-opened fd
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.
This can be used when QEMU is restricted from opening files, and
the management application opens files on QEMU's behalf.
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.
+static int qemu_dup(int fd, int flags)
+{
+ int ret;
+ int serrno;
+
+ if (flags & O_CLOEXEC) {
+ ret = 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 == -1 && errno == EINVAL) {
+ ret = dup(fd);
+ if (ret == -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 = fcntl(fd, F_GETFL);
+ if (eflags == -1) {
+ return -1;
+ }
+
+ if (((flags & O_RDWR) != (eflags & O_RDWR)) ||
+ ((flags & O_RDONLY) != (eflags & O_RDONLY)) ||
+ ((flags & O_WRONLY) != (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==0 on some platforms
(Linux), and ==1 on others (Hurd), and although POSIX recommends that
O_RDWR==(O_RDONLY|O_WRONLY) for any new systems, no one has really done
that except Hurd.
A correct way to write this is:
switch (flags & O_ACCMODE) {
case O_RDWR:
if ((eflags & O_ACCMODE) != O_RDWR) {
goto error;
break;
case O_RDONLY:
if ((eflags & O_ACCMODE) != O_RDONLY) {
goto error;
break;
case O_RDONLY:
if ((eflags & O_ACCMODE) != 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 = 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);
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)