On 06/22/2012 03:58 PM, Eric Blake wrote:
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?
Yes it sounds like this needs to be done. I'll fix this in v5 (as
discussed in the previous email).
+ 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).
I'll fix this in v5.
+
+ 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.
I'll remove this in v5.
+ (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.
I see your point. I'll call fcntl(F_GETFL) once to get the current
flags, determine what needs to be set on/off, and then call
fnctl(F_SETFL) once. In this case I won't be using fcntl_setfl()
anymore. Do you want me to drop the fcntl_setfl() changes I made?
Also, I noticed in the fnctl man page that F_SETFL: "On Linux this
command can change only the O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, and
O_NONBLOCK flags." So I'll only set/unset these flags.
+ /* 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:
}
Thanks, I'll update this in v5.
[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?
This is setting O_CLOEXEC for the inherited fd, and qemu_dup() is
setting O_CLOEXEC for the dup'd fd, so I think this needs to remain (and
use F_SETFD instead).
+ return -1;
+ }
+
+ return qemu_dup(fd, flags);
--
Regards,
Corey
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list