On Thu, 6 Sep 2012 14:54:35 +0100, "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list