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

[Eric Blake]

On 03/22/2011 05:29 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
> ---
>  po/POTFILES.in         |    1 +
>  src/Makefile.am        |   12 +++
>  src/fdstream.c         |  222 ++++++++++++++++++++++++++++++++++++------------
>  src/fdstream.h         |    5 +
>  src/lxc/lxc_driver.c   |    2 +-
>  src/qemu/qemu_driver.c |    2 +-
>  src/uml/uml_driver.c   |    2 +-
>  src/util/iohelper.c    |  208 +++++++++++++++++++++++++++++++++++++++++++++
>  src/xen/xen_driver.c   |    2 +-
>  9 files changed, 396 insertions(+), 60 deletions(-)
>  create mode 100644 src/util/iohelper.c

> @@ -206,6 +212,28 @@ static int virFDStreamFree(struct virFDStreamData *fdst)
>  {
>      int ret;
>      ret = VIR_CLOSE(fdst->fd);
> +    if (fdst->cmd) {
> +        ssize_t len = 1024;
> +        char buf[len];

This is a variable-sized buffer, which is not required by C89 (it was
added in C99)...

> +        int status;
> +        if ((len = saferead(fdst->errfd, buf, sizeof(buf)-1)) < 0)

so does sizeof(buf) give the right result on all compilers?  Or should
you change to char buf[1024]?

> +        } else if (status != 0) {
> +            if (buf[0] == '\0')
> +                streamsReportError(VIR_ERR_INTERNAL_ERROR,
> +                                   _("I/O helper exited with status %d"), status);

status includes normal exit and signals.  This should probably use
WIFEXITED and WEXITSTATUS to avoid printing values shifted by 8.  For
that matter, I just noticed that virCommandWait should probably be more
careful in how it interprets status.

> +        cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper",
> +                                   path,
> +                                   NULL);
> +        virCommandAddArgFormat(cmd, "%d", flags);
> +        virCommandAddArgFormat(cmd, "%d", mode);
> +        virCommandAddArgFormat(cmd, "%llu", offset);
> +        virCommandAddArgFormat(cmd, "%llu", length);
>  

> +        if (!cmd)
> +            goto error;

That only catches allocation failure, but not all other failures.  Since
virCommandRunAsync will also catch the same error, are these two lines
redundant?

>  
> -    if (fd < 0) {
> -        virReportSystemError(errno,
> -                             _("Unable to open stream for '%s'"),
> -                             path);
> -        return -1;
> -    }
> +        //virCommandDaemonize(cmd);

Any reason to keep this comment?  I don't see any reason to daemonize
the iohelper.

>  
> -    if (virFDStreamOpen(st, fd) < 0)
> +    if (virFDStreamOpenInternal(st, fd, cmd, errfd, length) < 0)
>          goto error;
>  
>      return 0;
>  
>  error:
> +    virCommandFree(cmd);

If virFDStreamOpenInternal fails, but we've already spawned the child
process, virCommandFree won't reap that process.

> +    VIR_FORCE_CLOSE(fds[0]);
> +    VIR_FORCE_CLOSE(fds[1]);
>      VIR_FORCE_CLOSE(fd);

Then again, once the fds are closed, the child should eventually die
naturally from EOF or SIGPIPE.  But it does raise the question of
whether this cleanup need any reorganization to reap any child process
on error.

> +int virFDStreamCreateFile(virStreamPtr st,
> +                          const char *path,
> +                          unsigned long long offset,
> +                          unsigned long long length,
> +                          int flags,
> +                          mode_t mode)
> +{
> +    return virFDStreamOpenFileInternal(st, path, offset, length,
flags, mode);

Should this fail if (flags&O_CREAT) == 0?

> +++ b/src/util/iohelper.c

> +    if (offset) {
> +        if (lseek(fd, offset, SEEK_SET) < 0) {
> +            virReportSystemError(errno, _("Unable to seek %s to %llu"),
> +                                 path, offset);
> +            goto cleanup;
> +        }
> +    }
> +

> +
> +    offset = 0;

Did you really intend to zero out the offset here?

> +int main(int argc, char **argv)
> +{
> +    const char *path;
> +    const char *op;

Dead variable.

> +    virErrorPtr err;
> +    unsigned long long offset;
> +    unsigned long long length;
> +    int flags;
> +    int mode;
> +
> +    if (setlocale(LC_ALL, "") == NULL ||
> +        bindtextdomain(PACKAGE, LOCALEDIR) == NULL ||
> +        textdomain(PACKAGE) == NULL) {
> +        fprintf(stderr, _("%s: initialization failed\n"), argv[0]);
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    if (virThreadInitialize() < 0 ||
> +        virErrorInitialize() < 0 ||
> +        virRandomInitialize(time(NULL) ^ getpid())) {
> +        fprintf(stderr, _("%s: initialization failed\n"), argv[0]);
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    if (argc != 6) {
> +        fprintf(stderr, _("%s: syntax FILENAME FLAGS MODE OFFSET LENGTH\n"), argv[0]);
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    path = argv[1];
> +    op = argv[2];

Dead assignment.

> +
> +    if ((flags & O_RDWR) == O_RDWR) {
> +        exit(EXIT_FAILURE);
> +    }

Won't work.  It should be:

if ((flags & O_ACCMODE) == O_RDWR)

But runIO makes the same check, so you could just omit it from here.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 615 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110322/50e5cf8c/attachment-0001.sig>