9:24 a.m.
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