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

[lhuang]

On 07/06/2015 01:38 PM, Martin Kletzander wrote:
> 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 at redhat.com>
>>>> ---
>>>> 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.
>

Okay, got it, thanks a lot for your helps.

BR,
Luyao

> 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 at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/libvir-list
>>