Re: [PATCH 03/10] qemu: Refactor creation of shared memory device commandline

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Mon, Jul 06, 2015 at 10:23:59AM +0800, lhuang wrote:
> On 07/03/2015 08:56 PM, Martin Kletzander wrote:
> > On Wed, Jun 17, 2015 at 11:56:14AM +0800, Luyao Huang wrote:
> > > Rename qemuBuildShmemDevCmd to qemuBuildShmemDevStr and change the
> > > return type so that it can be reused in the device hotplug code later.
> > >
> > > And split the chardev creation part in a new function
> > > qemuBuildShmemBackendStr for reused in the device hotplug code later.
> > >
> > > Signed-off-by: Luyao Huang <lhuang@xxxxxxxxxx>
> > > ---
> > > src/qemu/qemu_command.c | 70
> > > +++++++++++++++++++++++++++----------------------
> > > src/qemu/qemu_command.h |  7 +++++
> > > 2 files changed, 45 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > index 636e040..0414f77 100644
> > > --- a/src/qemu/qemu_command.c
> > > +++ b/src/qemu/qemu_command.c
> > > @@ -8433,9 +8433,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
> > >    return ret;
> > > }
> > >
> > > -static int
> > > -qemuBuildShmemDevCmd(virCommandPtr cmd,
> > > -                     virDomainDefPtr def,
> > > +char *
> > > +qemuBuildShmemDevStr(virDomainDefPtr def,
> > >                     virDomainShmemDefPtr shmem,
> > >                     virQEMUCapsPtr qemuCaps)
> > > {
> > > @@ -8489,14 +8488,38 @@ qemuBuildShmemDevCmd(virCommandPtr cmd,
> > >    if (virBufferCheckError(&buf) < 0)
> > >        goto error;
> > >
> > > -    virCommandAddArg(cmd, "-device");
> > > -    virCommandAddArgBuffer(cmd, &buf);
> > > -
> > > -    return 0;
> > > +    return virBufferContentAndReset(&buf);
> >
> > You should be able to just unconditionally do
> > return virBufferContentAndReset() here since it returns NULL if
> > there's a buf->error.
> >
> > ACK with that changed.
>
> Right, i forgot that, thanks a lot for your review

Sorry, you cannot, there's a "goto error;" from some part of the code
where there is no buf->error set, so no change to this, you were
right.

No need to resend these first three, I'll push whatever is OK to go in
after I finish the review, and let you know what needs work.

Thanks,
Martin

> Luyao
>
> > > error:
> > >    virBufferFreeAndReset(&buf);
> > > -    return -1;
> > > +    return NULL;
> > > +}
> > > +
> > > +char *
> > > +qemuBuildShmemBackendStr(virDomainShmemDefPtr shmem,
> > > +                         virQEMUCapsPtr qemuCaps)
> > > +{
> > > +    char *devstr = NULL;
> > > +    virDomainChrSourceDef source = {
> > > +        .type = VIR_DOMAIN_CHR_TYPE_UNIX,
> > > +        .data.nix = {
> > > +            .path = shmem->server.path,
> > > +            .listen = false,
> > > +        }
> > > +    };
> > > +
> > > +    if (!shmem->server.path &&
> > > +        virAsprintf(&source.data.nix.path,
> > > +                    "/var/lib/libvirt/shmem-%s-sock",
> > > +                    shmem->name) < 0)
> > > +        return NULL;
> > > +
> > > +    devstr = qemuBuildChrChardevStr(&source, shmem->info.alias,
> > > qemuCaps);
> > > +
> > > +    if (!shmem->server.path)
> > > +        VIR_FREE(source.data.nix.path);
> > > +
> > > +    return devstr;
> > > }
> > >
> > > static int
> > > @@ -8505,35 +8528,18 @@ qemuBuildShmemCommandLine(virCommandPtr cmd,
> > >                          virDomainShmemDefPtr shmem,
> > >                          virQEMUCapsPtr qemuCaps)
> > > {
> > > -    if (qemuBuildShmemDevCmd(cmd, def, shmem, qemuCaps) < 0)
> > > +    char *devstr = NULL;
> > > +
> > > +    if (!(devstr = qemuBuildShmemDevStr(def, shmem, qemuCaps)))
> > >        return -1;
> > > +    virCommandAddArgList(cmd, "-device", devstr, NULL);
> > > +    VIR_FREE(devstr);
> > >
> > >    if (shmem->server.enabled) {
> > > -        char *devstr = NULL;
> > > -        virDomainChrSourceDef source = {
> > > -            .type = VIR_DOMAIN_CHR_TYPE_UNIX,
> > > -            .data.nix = {
> > > -                .path = shmem->server.path,
> > > -                .listen = false,
> > > -            }
> > > -        };
> > > -
> > > -        if (!shmem->server.path &&
> > > -            virAsprintf(&source.data.nix.path,
> > > -                        "/var/lib/libvirt/shmem-%s-sock",
> > > -                        shmem->name) < 0)
> > > +        if (!(devstr = qemuBuildShmemBackendStr(shmem, qemuCaps)))
> > >            return -1;
> > >
> > > -        devstr = qemuBuildChrChardevStr(&source,
> > > shmem->info.alias, qemuCaps);
> > > -
> > > -        if (!shmem->server.path)
> > > -            VIR_FREE(source.data.nix.path);
> > > -
> > > -        if (!devstr)
> > > -            return -1;
> > > -
> > > -        virCommandAddArg(cmd, "-chardev");
> > > -        virCommandAddArg(cmd, devstr);
> > > +        virCommandAddArgList(cmd, "-chardev", devstr, NULL);
> > >        VIR_FREE(devstr);
> > >    }
> > >
> > > diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> > > index 0fc59a8..73f24dc 100644
> > > --- a/src/qemu/qemu_command.h
> > > +++ b/src/qemu/qemu_command.h
> > > @@ -194,6 +194,13 @@ int
> > > qemuBuildRNGBackendProps(virDomainRNGDefPtr rng,
> > >                             const char **type,
> > >                             virJSONValuePtr *props);
> > >
> > > +char *qemuBuildShmemDevStr(virDomainDefPtr def,
> > > +                           virDomainShmemDefPtr shmem,
> > > +                           virQEMUCapsPtr qemuCaps);
> > > +char *qemuBuildShmemBackendStr(virDomainShmemDefPtr shmem,
> > > +                               virQEMUCapsPtr qemuCaps);
> > > +
> > > +
> > > int qemuOpenPCIConfig(virDomainHostdevDefPtr dev);
> > >
> > > /* Legacy, pre device support */
> > > --
> > > 1.8.3.1
> > >
> > > --
> > > libvir-list mailing list
> > > libvir-list@xxxxxxxxxx
> > > https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list