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

[Eric Blake]

On 03/23/2011 11:36 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 +213,35 @@ static int virFDStreamFree(struct virFDStreamData *fdst)
>  {
>      int ret;
>      ret = VIR_CLOSE(fdst->fd);
> +    if (fdst->cmd) {
> +        char buf[1024];
> +        ssize_t len;
> +        int status;
> +        if ((len = saferead(fdst->errfd, buf, sizeof(buf)-1)) < 0)
> +            buf[0] = '\0';
> +        else
> +            buf[len] = '\0';
> +
> +        if (virCommandWait(fdst->cmd, &status) < 0) {
> +            ret = -1;
> +        } else if (status != 0) {
> +            if (buf[0] == '\0') {
> +                if (WIFEXITED(status)) {
> +                    streamsReportError(VIR_ERR_INTERNAL_ERROR,
> +                                       _("I/O helper exited with status %d"),
> +                                       WEXITSTATUS(status));
> +                } else {
> +                    streamsReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                       _("I/O helper exited abnormally"));

Is it worth using virCommandTranslateStatus here to come up with a
better message?  That depends on this unreviewed patch:
https://www.redhat.com/archives/libvir-list/2011-March/msg01119.html

If you push this one first, I can rebase that one.

> @@ -346,6 +419,13 @@ int virFDStreamOpen(virStreamPtr st,
>  }
>  
>  
> +int virFDStreamOpen(virStreamPtr st,
> +                    int fd)
> +{
> +    return virFDStreamOpenInternal(st, fd, NULL, -1, 0);
> +}

Hmm.  This just blindly uses fd as a stream, even if it is a regular
file.  And my fd: migration code (eek - I really need to repost that
soon, after running more testing on it) sometimes passes a regular file;
and not just any regular file, but a fd that might have been opened with
virFileOpenAs to bypass NFS root-squash limitations.  You _want_ to use
the iohelper for regular file fds.  And thinking about it more...

> +static int
> +virFDStreamOpenFileInternal(virStreamPtr st,
> +                            const char *path,
> +                            unsigned long long offset,
> +                            unsigned long long length,
> +                            int flags,
> +                            int mode)
>  {
...
> +    if (flags & O_CREAT)
> +        fd = open(path, flags, mode);
> +    else
> +        fd = open(path, flags);
> +    if (fd < 0) {
>          virReportSystemError(errno,

> @@ -440,64 +530,95 @@ int virFDStreamOpenFile(virStreamPtr st,
>      if ((st->flags & VIR_STREAM_NONBLOCK) &&
>          (!S_ISCHR(sb.st_mode) &&
>           !S_ISFIFO(sb.st_mode))) {
...

> +
> +        VIR_FORCE_CLOSE(fd);

This block of virFDStreamOpenFile needs to instead live in
virFDStreamOpen, and instead of passing a file name to the iohelper
process (which is slightly racy, because the file could change between
the two calls to open(), and only works for non-root-squash files), you
should instead pass an open fd to the iohelper process (along with a
string representation of which fd the iohelper process should have
expected to inherit).  That is, the iohelper should _not_ call open(),
but should already be handed the fd in question from the parent process.

Perhaps you might _also_ want to change the signature of virFDStreamOpen
to take an optional const char * name of the fd, which if non-NULL can
be used for more informative error messages, but which is not essential.

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

Given the above discussion, you don't need to pass flags or mode, but do
need to pass offset and length as well as a new fd argument, and pass an
empty string when path is otherwise unknown (since you can't safely pass
NULL).  Hmm, even offset might not be necessary (the iohelper could use
lseek to learn the current position); but length is necessary to cap io
at the right limits.

> +
> +        if (virCommandRunAsync(cmd, &pid) < 0)
> +            goto error;
> +
...
>  
>  error:
> +    if (pid)
> +        kill(SIGTERM, pid);

You could replace the two line if and kill() with a one-liner
virCommandAbort(cmd)...

> +    virCommandFree(cmd);

if my patch goes in first (this is a case where virCommandFree won't do
it for you, because you grabbed the pid during virCommandRunAsync).
https://www.redhat.com/archives/libvir-list/2011-March/msg01120.html

Now, all that said, I don't have any technical objections to this patch
as-is (virFDStreamOpen is just as broken before and after your patch if
I get my fd: migration code working).  In other words, all my ideas
about converting iohelper to use an inherited fd could be done as an
incremental followup patch rather than holding this one up any longer;
that is, I'd rather see file upload/download get into 0.9.0 rather than
miss out on it just because there's room for further improvement at a
later date.  So you have my:

ACK

and it's up to you whether to use that ACK and push now, or do a v4 with
the improved iohelper via fd inheritance.

-- 
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: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110324/38176a7e/attachment-0001.sig>