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

[Daniel P. Berrange]

On Wed, Aug 08, 2012 at 03:03:51PM +0100, Daniel P. Berrange wrote:
> 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.

Actually I take this back. You are doing what I originally
suggested you do, which is correct.  Looking in $PATH would
be wrong since it might find a proxy that's from a different
version of QEMU. We do need to take account of fact that
QEMU might be in $prefix/libexec, instead of $prefix/bin.

Also Unfortunately dirname is not thread-safe since it can
return a statically allocated string - which must not
be free()d, so we can't use that.

So I think I'd build the path slightly differently:

  char *tmp, *prefix;
  if (!(prefix = strdup(emulator))) {
     virReportOOMError();
     return -1;
  }
  if (tmp = strstr(prefix, "/bin/") ||
      tmp = strstr(prefix, "/libexec/") {
    *tmp = '\0';
  } else {
     virReportError(VIR_ERR_INTERNAL_ERROR,
                   _("Unable to determine prefix from %s"),
                    emulator);
     VIR_FREE(prefix);
  }
  if (virAsprintf(&path, "%s/bin/" HELPER, prefix) < 0) {
     VIR_FREE(prefix);
     return -1;
  }
  VIR_FREE(prefix);

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