Re: [Qemu-devel] [PATCH v4 7/7] osdep: Enable qemu_open to dup pre-opened fd

[Corey Bryant]

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