On 10/18/2016 10:57 AM, Pavel Hrdina wrote:
> On Tue, Oct 18, 2016 at 10:37:37AM -0400, John Ferlan wrote:
>>
>>
>> On 10/18/2016 09:58 AM, Pavel Hrdina wrote:
>>> There was inconsistency between alias used to create tls-creds-x509
>>> object and alias used to link that object to chardev while hotpluging.
>>>
>>> In XML we have for example alias "serial0", but on qemu command line we
>>> generate "charserial0".
>>>
>>> The issue was that code, that creates QMP command to hotplug chardev
>>> devices uses only the second alias "charserial0" and that alias is also
>>> used to link the tls-creds-x509 object.
>>
>> Which two objects used the same ID in hotplug?
>>
>> tlsProps would use obj%s_tls0
>>
>> and this changes it to be essentially objchar%s_tls0
>>
>> Essentially I'm trying to figure out from the commit message prior to
>> this patch what was created incorrectly.
> 
> The issue is that while hotpluging chardev device those QMP command are 
> created
> as I've described:
> 
> {
>     "execute":"object-add",
>     "arguments": {
>         "qom-type":"tls-creds-x509",
>         "id":"objchannel3_tls0",
>               ^^^^^^^^^^^^^^^^
>         "props": {
>             "dir":"/etc/pki/libvirt-chardev",
>             "endpoint":"server",
>             "verify-peer":false
>         }
>     },
>     "id":"libvirt-29"
> }
> 
> {
>     "execute":"chardev-add",
>     "arguments": {
>         "id":"charchannel3",
>         "backend": {
>             "type":"socket",
>             "data": {
>                 "addr": {
>                     "type":"inet",
>                     "data": {
>                         "host":"localhost",
>                         "port":"3344"
>                     }
>                 },
>                 "wait":false,
>                 "telnet":false,
>                 "server":true,
>                 "tls-creds":"objcharchannel3_tls0"
>                              ^^^^^^^^^^^^^^^^^^^^
>             }
>         }
>     },
>     "id":"libvirt-30"
> }
> 
> And as you can see the alias used to create tls-creds-x509 object is different
> than the one used while attaching chardev.  That's because function
> qemuMonitorJSONAttachCharDevCommand() takes as first argument "charchannel3"
> instead of "channel3".  So the logical solution is to use "charchannel3" as 
> base
> while creating "obj%s_tls0" alias.

Ah... OK I see...  I wasn't visualizing the chardev-add from the commit
message.

tks -

John

> 
>>> This patch unifies the aliases for tls-creds-x509 to be always generated
>>> from "charserial0".
>>>
>>> Signed-off-by: Pavel Hrdina <phrd...@redhat.com>
>>> ---
>>>  src/qemu/qemu_command.c                                          | 4 ++--
>>>  src/qemu/qemu_hotplug.c                                          | 9 
>>> +++++++--
>>>  .../qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args          | 4 ++--
>>>  .../qemuxml2argv-serial-tcp-tlsx509-chardev.args                 | 4 ++--
>>>  4 files changed, 13 insertions(+), 8 deletions(-)
>>>
>>
>>
>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 74f65c0..8d87e69 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -4949,10 +4949,10 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
>>>              if (qemuBuildTLSx509CommandLine(cmd, 
>>> cfg->chardevTLSx509certdir,
>>>                                              dev->data.tcp.listen,
>>>                                              cfg->chardevTLSx509verify,
>>> -                                            alias, qemuCaps) < 0)
>>> +                                            charAlias, qemuCaps) < 0)
>>>                  goto error;
>>>  
>>> -            if (!(objalias = qemuAliasTLSObjFromChardevAlias(alias)))
>>> +            if (!(objalias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>>>                  goto error;
>>>              virBufferAsprintf(&buf, ",tls-creds=%s", objalias);
>>>              VIR_FREE(objalias);
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index c2ba935..e39bd8b 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -1738,7 +1738,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
>>>                                           &tlsProps) < 0)
>>>              goto cleanup;
>>>  
>>> -        if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(chr->info.alias)))
>>> +        if (!(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>>>              goto cleanup;
>>>          dev->data.tcp.tlscreds = true;
>>>      }
>>> @@ -4387,6 +4387,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>>>      virDomainChrDefPtr tmpChr;
>>>      char *objAlias = NULL;
>>>      char *devstr = NULL;
>>> +    char *charAlias = NULL;
>>>  
>>>      if (!(tmpChr = virDomainChrFind(vmdef, chr))) {
>>>          virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>>> @@ -4397,11 +4398,14 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr 
>>> driver,
>>>      if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) 
>>> < 0)
>>>          goto cleanup;
>>>  
>>> +    if (virAsprintf(&charAlias, "char%s", tmpChr->info.alias) < 0)
>>> +        goto cleanup;
>>> +
>>
>> This would seemingly need to go after the subsequent line...  Although I
>> think the subsequent line gets removed if use a qemu_alias.c helper.
> 
> The helper would be called here, so I'll move it after that assert.
> 
> Thanks
> 
> Pavel
> 
>>
>>>      sa_assert(tmpChr->info.alias);
>>>  
>>>      if (tmpChr->source.type == VIR_DOMAIN_CHR_TYPE_TCP &&
>>>          cfg->chardevTLS &&
>>> -        !(objAlias = qemuAliasTLSObjFromChardevAlias(tmpChr->info.alias)))
>>> +        !(objAlias = qemuAliasTLSObjFromChardevAlias(charAlias)))
>>>          goto cleanup;
>>>  
>>>      if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0)
>>> @@ -4427,6 +4431,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>>>   cleanup:
>>>      qemuDomainResetDeviceRemoval(vm);
>>>      VIR_FREE(devstr);
>>> +    VIR_FREE(charAlias);
>>>      virObjectUnref(cfg);
>>>      return ret;
>>>  
>>> diff --git 
>>> a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
>>>  
>>> b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
>>> index f521e33..003d11d 100644
>>> --- 
>>> a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
>>> +++ 
>>> b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-verify.args
>>> @@ -25,9 +25,9 @@ server,nowait \
>>>  -chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\
>>>  localport=1111 \
>>>  -device isa-serial,chardev=charserial0,id=serial0 \
>>> --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\
>>> +-object 
>>> tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\
>>>  endpoint=client,verify-peer=yes \
>>>  -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\
>>> -tls-creds=objserial1_tls0 \
>>> +tls-creds=objcharserial1_tls0 \
>>>  -device isa-serial,chardev=charserial1,id=serial1 \
>>>  -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
>>> diff --git 
>>> a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args 
>>> b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args
>>> index 4c8c23e..b456cce 100644
>>> --- a/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args
>>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev.args
>>> @@ -25,9 +25,9 @@ server,nowait \
>>>  -chardev udp,id=charserial0,host=127.0.0.1,port=2222,localaddr=127.0.0.1,\
>>>  localport=1111 \
>>>  -device isa-serial,chardev=charserial0,id=serial0 \
>>> --object tls-creds-x509,id=objserial1_tls0,dir=/etc/pki/libvirt-chardev,\
>>> +-object 
>>> tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/libvirt-chardev,\
>>>  endpoint=client,verify-peer=no \
>>>  -chardev socket,id=charserial1,host=127.0.0.1,port=5555,\
>>> -tls-creds=objserial1_tls0 \
>>> +tls-creds=objcharserial1_tls0 \
>>>  -device isa-serial,chardev=charserial1,id=serial1 \
>>>  -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
>>>

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

Reply via email to