[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: Generate agent socket path if missing



On 23.04.2013 17:40, Jiri Denemark wrote:
> On Tue, Apr 09, 2013 at 19:05:28 +0200, Michal Privoznik wrote:
>> It's not desired to force users imagine path for a socket they
>> are not even supposed to connect to. On the other hand, we
>> already have a release where the qemu agent socket path is
>> exposed to XML, so we cannot silently drop it from there.
>> The new path is generated in form:
>>
>> $LOCALSTATEDIR/lib/libvirt/qemu/channel/target/$domain.$name
>> ---
>>  libvirt.spec.in        |  1 +
>>  src/Makefile.am        |  1 +
>>  src/conf/domain_conf.c | 34 ++++++++++++++++------------------
>>  src/qemu/qemu_domain.c | 16 ++++++++++++++++
>>  4 files changed, 34 insertions(+), 18 deletions(-)
> ...
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 3d6eef4..967890f 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -738,6 +738,22 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>>          (def->os.arch == VIR_ARCH_S390 || def->os.arch == VIR_ARCH_S390X))
>>          dev->data.chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO;
>>  
>> +    /* auto generate unix socket path */
>> +    if (dev->type == VIR_DOMAIN_DEVICE_CHR &&

1: ^^

>> +        dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
>> +        dev->data.chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
>> +        dev->data.chr->source.type == VIR_DOMAIN_CHR_TYPE_UNIX &&
>> +        !dev->data.chr->source.data.nix.path &&
>> +        (cfg || (driver && (cfg = virQEMUDriverGetConfig(driver))))) {
> 
> I think this should fail if path is NULL and
> (cfg || (driver && (cfg = virQEMUDriverGetConfig(driver)))) is not true.
> 
> On the other hand, do we actually need to check for this? Aren't both
> cfg and driver guaranteed to be non-NULL at this point?

And actually as Eric pointed out, driver can be NULL and so does cfg.
The only way where cfg already is not NULL is for (dev->type ==
VIR_DOMAIN_DEVICE_DISK) in which case the [1] condition is false. So in
fact, there is no way for cfg to be other than NULL, hence I can drop
check for cfg being not NULL:
    if (dev->type = VIR_DOMAIN_DEVICE_CHR && ...
        (driver && (cfg = virQEMUDriverGetConfig(driver)))) {

> 
>> +
>> +        if (virAsprintf(&dev->data.chr->source.data.nix.path,
>> +                        "%s/channel/target/%s.%s",
>> +                        cfg->libDir, def->name,
>> +                        dev->data.chr->target.name) < 0)
>> +            goto no_memory;
>> +        dev->data.chr->source.data.nix.listen = true;
>> +    }
>> +
>>      ret = 0;
>>  
>>  cleanup:
> 
> Jirka
> 

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]