[libvirt] [PATCH V3] Add proxy FS support to libvirt

[Daniel P. Berrange]

On Sun, Aug 05, 2012 at 10:04:35AM +0530, M. Mohan Kumar wrote:
> @@ -2575,9 +2577,43 @@ error:
>      return NULL;
>  }
>  
> +/*
> + * Invokes the Proxy Helper with one of the socketpair as its parameter
> + *
> + */
> +static int qemuInvokeProxyHelper(const char *emulator, int sock, const char *path)
> +{
> +#define HELPER "virtfs-proxy-helper"
> +    int ret_val, status;
> +    virCommandPtr cmd;
> +    char *helper, *dname;
> +
> +    dname = dirname(strdup(emulator));

Missing check for NULL from strdup()

> +    if (virAsprintf(&helper, "%s/%s", dname, HELPER) < 0) {
> +        VIR_FREE(dname);
> +        virReportOOMError();
> +        return -1;

You must VIR_FORCE_CLOSE(sock) here, since other exit
paths will close it

> +    }

I think this is slightly bogus - and it'd be  better to
do  virFindFileInPath(HELPER), and if that doesn't work
then also look in /usr/libexec/$HELPER.

> +
> +    cmd = virCommandNewArgList(helper, NULL);
> +    virCommandAddArg(cmd, "-f");
> +    virCommandAddArgFormat(cmd, "%d", sock);
> +    virCommandAddArg(cmd, "-p");
> +    virCommandAddArgFormat(cmd, "%s", path);
> +    virCommandTransferFD(cmd, sock);
> +    virCommandDaemonize(cmd);
> +    ret_val = virCommandRun(cmd, &status);
> +    if (ret_val < 0)
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                            _("%s can't execute"), helper);
> +    virCommandFree(cmd);
> +    VIR_FREE(helper);
> +    VIR_FREE(dname);
> +    return ret_val;
> +}
>  
>  char *qemuBuildFSStr(virDomainFSDefPtr fs,
> -                     virBitmapPtr qemuCaps ATTRIBUTE_UNUSED)
> +                     virBitmapPtr qemuCaps ATTRIBUTE_UNUSED, int qemuSocket)
>  {
>      virBuffer opt = VIR_BUFFER_INITIALIZER;
>      const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver);
> @@ -2626,6 +2662,10 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs,
>      }
>  
>      virBufferAsprintf(&opt, ",id=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias);
> +
> +    if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PROXY)
> +        virBufferAsprintf(&opt, ",sock_fd=%d", qemuSocket);
> +
>      virBufferAsprintf(&opt, ",path=%s", fs->src);
>  
>      if (fs->readonly) {
> @@ -5081,10 +5121,35 @@ qemuBuildCommandLine(virConnectPtr conn,
>      if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV)) {
>          for (i = 0 ; i < def->nfss ; i++) {
>              char *optstr;
> +            int sockets[2] = {-1, -1};
>              virDomainFSDefPtr fs = def->fss[i];
>  
> +            /*
> +             * If its a proxy FS, we need to create a socket pair
> +             * and invoke proxy_helper
> +             */
> +            if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PROXY) {
> +                if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV_PROXY) < 0) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                        _("proxy helper not supported"));
> +                    goto error;
> +                }
> +                /* create a socket pair */
> +                if (socketpair(PF_UNIX, SOCK_STREAM, 0, sockets) < 0) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

Use 'virReportSystemError(errno...'

> +                        _("socketpair failed"));
> +                    goto error;
> +                }
> +                virCommandTransferFD(cmd, sockets[1]);
> +                if (qemuInvokeProxyHelper(def->emulator, sockets[0],
> +                                    fs->src) < 0) {
> +                    VIR_FORCE_CLOSE(sockets[0]);
> +                    VIR_FORCE_CLOSE(sockets[1]);

You've already given sockets[i] to virCommandPtr, so that
will take care of closing it when virCommandFree is called.

Liekwise qemuInvokeProxyHelper passes sockets[0] to 
another virCommandPtr so it will get closed on some, but
not all exit paths.

So remove both these VIR_FORCE_CLOSE lines

> +                    goto error;
> +                }
> +            }
>              virCommandAddArg(cmd, "-fsdev");
> -            if (!(optstr = qemuBuildFSStr(fs, qemuCaps)))
> +            if (!(optstr = qemuBuildFSStr(fs, qemuCaps, sockets[1])))
>                  goto error;
>              virCommandAddArg(cmd, optstr);
>              VIR_FREE(optstr);

Regards,
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 :|