[libvirt] [PATCH 1/5] Enhance the streams helper to support plain file I/O

[Daniel P. Berrange]

On Tue, Mar 15, 2011 at 09:53:40AM -0600, Eric Blake wrote:
> On 03/15/2011 06:30 AM, Daniel P. Berrange wrote:
> > The O_NONBLOCK flag doesn't work as desired on plain files
> > or block devices. Introduce an I/O helper program that does
> > the blocking I/O operations, communicating over a pipe that
> > can support O_NONBLOCK
> > 
> > * src/fdstream.c, src/fdstream.h: Add non-blocking I/O
> >   on plain files/block devices
> > * src/Makefile.am, src/util/iohelper.c: I/O helper program
> > * src/qemu/qemu_driver.c, src/lxc/lxc_driver.c,
> >   src/uml/uml_driver.c, src/xen/xen_driver.c: Update for
> >   streams API change
> 
> > @@ -206,6 +212,25 @@ static int virFDStreamFree(struct virFDStreamData *fdst)
> >  {
> >      int ret;
> >      ret = VIR_CLOSE(fdst->fd);
> > +    if (fdst->cmd) {
> > +        int status;
> > +        if (virCommandWait(fdst->cmd, &status) < 0)
> > +            ret = -1;
> > +        if (status != 0) {
> > +            ssize_t len = 1024;
> > +            char buf[len];
> > +            if ((len = saferead(fdst->errfd, buf, len)) < 0) {
> 
> Is this safe to read from errfd after you've already waited for the
> child to exit?  After all, exit() is allowed to discard unread data from
> a pipe, and once the child is exited, the parent may get EOF instead of
> that data.  Conversely, can the child block trying to write into errfd,
> and thus never reach the exit, so that you have a deadlock where the
> parent is waiting without reading?  I think you have to read errfd prior
> to calling virCommandWait.

Empirically it worked fine, but I've reordered it now.

> > -    if ((fd  = open(path, flags)) < 0) {
> > +    if (flags & O_CREAT)
> > +        fd = open(path, flags, mode);
> > +    else
> > +        fd = open(path, flags);
> 
> Technically, it's safe to call fd = open(path, flags, mode) even when
> flags does not contain O_CREAT, but I'm fine with keeping this if statement.

Yes, I think it is nice  to be explicit here.

> > +        const char *opname;
> > +        int childfd;
> > +        char offsetstr[100];
> > +        char lengthstr[100];
> > +
> > +        snprintf(offsetstr, sizeof(offsetstr), "%llu", offset);
> > +        snprintf(lengthstr, sizeof(lengthstr), "%llu", length);
> 
> 100 is too big.  Use this for a tighter bound:
> 
> #include "intprops.h"
> 
> char offsetstr[INT_BUFSIZE_BOUND(offset)];
> 
> But see below for an alternate suggestion that loses these variables
> altogether.

I switched to virCommandAddArgFormat, since I also now have
two more integer args to pass

> > +
> > +        if (flags == O_RDONLY) {
> > +            opname = "read";
> > +        } else if (flags == O_WRONLY) {
> > +            opname = "write";
> > +        } else if (flags == (O_WRONLY|O_CREAT)) {
> > +            opname = "create";
> > +        } else {
> > +            streamsReportError(VIR_ERR_INTERNAL_ERROR,
> > +                               _("Non-blocking I/O is not supported on %s with flags %d"),
> 
> Okay, so you insist on a specific subset of flags.  But what about flags
> like O_CLOEXEC?  Should you only be comparing against the bitmask of
> flags that you actually care about, while allowing other flags as
> needed?  Also, should you support O_EXCL?  I guess we can add support
> for more flags at the point where we add more clients that want to use
> those flags.

I've killed this. Instead of passing an 'opname' to the command helper,
I just pass the raw int value of 'flags' and 'mode' as parameters.

> > +
> > +        if (!cmd)
> > +            goto error;
> > +
> > +        //virCommandDaemonize(cmd);
> > +        if (flags == O_RDONLY) {
> > +            childfd = fds[1];
> > +            fd = fds[0];
> > +            virCommandSetOutputFD(cmd, &childfd);
> > +        } else {
> > +            childfd = fds[0];
> > +            fd = fds[1];
> > +            virCommandSetInputFD(cmd, childfd);
> 
> Should we also be setting FD_CLOEXEC on the side of the pipe not given
> to the child?  (I really need to find time to work on O_CLOEXEC support
> in gnulib, then do an audit over all of libvirt to take advantage of
> atomic CLOEXEC support by using things like pipe2).

We've not bothered with CLOEXEC anywhere else in our code really,
since we loop over all FDs and explicitly close them.

> > +    if (fd < 0) {
> > +        virReportSystemError(errno, _("Unable to open %s"), path);
> > +        goto cleanup;
> > +    }
> > +
> > +#if 0
> > +    if (length && (flags & O_CREAT)) {
> > +        if (ftruncate(fd, length) < 0) {
> 
> Same comment about O_TRUNC support.

I removed this unused code - it was from an earlier idea that didn't
play out.

> > +    if (argc != 5) {
> > +        fprintf(stderr, _("%s: syntax FILENAME OPERATION OFFSET LENGTH\n"), argv[0]);
> > +        exit(EXIT_FAILURE);
> > +    }
> 
> Do we want --help/--version support (and a man page), or is this program
> considered internal enough to not need the public-facing documentation?

It isn't intended for end users to ever run this.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|