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

[M. Mohan Kumar]

On Thu, 6 Sep 2012 14:54:35 +0100, "Daniel P. Berrange" <berrange at redhat.com> wrote:
> >  docs/formatdomain.html.in    |    2 +
> >  src/conf/domain_conf.c       |    3 +-
> >  src/conf/domain_conf.h       |    2 +-
> 
> Opps, forgot to mention in previous reviews that you must
> update docs/schemas/domaincommon.rng to list the new attribute
> 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 5472267..88b7e87 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -144,7 +144,6 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
> >                "ich9-ahci",
> >                "no-acpi",
> >                "fsdev-readonly",
> > -
> >                "virtio-blk-pci.scsi", /* 80 */
> >                "blk-sg-io",
> >                "drive-copy-on-read",
> 
> Bogus whitespace removal
> 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 4ca3047..3f78635 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -46,6 +46,7 @@
> >  #include <sys/utsname.h>
> >  #include <sys/stat.h>
> >  #include <fcntl.h>
> > +#include <libgen.h>
> 
> You don't use any functions from this header, so it can be removed.
>
> > @@ -2632,9 +2634,59 @@ 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, *dp;
> > +
> > +    dp = strdup(emulator);
> > +    if (!dp) {
> > +        virReportOOMError();
> > +        return -1;
> 
> This doesn't close 'sock' like other error paths do

I will fix these issues.

> Hmm, right here we are in the middle of constructing the command line
> args for QEMU. We should not be launching at processes at this point,
> not least since we could be running this from the test suite.
>
ie we should invoke during qemuProcessStart(), I will modify.
 
> Also you don't appear to have any code to tell the daemonized proxy
> to shutdown when QEMU is stopped ?
>
When QEMU terminates (socket closed), proxy helper also terminates 
(reading from socket fails and it terminates).
 
> We really need to push the invocation of the proxy up into the code
> which actually launches QEMU, and merely have this part construct the
> command line arguments.
>
Ok, I will apply these changes and change next version
 
Thanks for the review