[libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device

[Ján Tomko]

On 01/21/2015 04:10 AM, lhuang wrote:
> 
> On 01/21/2015 03:00 AM, John Ferlan wrote:
>>
>> On 12/09/2014 01:48 AM, Luyao Huang wrote:
>>> Add a func just check the base target type which
>>> qemu support. But i still doubt this will be useful
>>> , we already have a check when we try to start the
>>> vm. And this check only check the target type, and
>>> the other things will be done in virDomainChrDefParseXML.
>>>
>> The commit message needs some massaging.
>>
>> Essentially, this patch fixes the "condition" where if a guest has been
>> started and someone uses attach-device with (or without) the --config
>> option, then these checks will avoid the "next" guest being modified,
>> correct?
> 
> Right
>> This will also cause an error earlier that patch 1/2 as
>> qemuDomainChrInsert is called in the path before qemuDomainAttachDeviceLive
>>
>>
> 
> Yes and if this patch have been pushed then the patch 1/2 will be a patch for
> improving exist code.

ChrInsert is called after qemuBuildConsoleChrDeviceStr in AttachDevice. We
should error out earlier and include the first patch too.

>>> Signed-off-by: Luyao Huang <lhuang at redhat.com>
>>> ---
>>>   src/qemu/qemu_hotplug.c | 64
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 64 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>> index b9a0cee..fe91ec7 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -1384,10 +1384,74 @@ int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr
>>> driver,
>>>     }
>>>   +static int
>>> +qemuDomainChrCheckDefSupport(virDomainChrDefPtr chr)
>>> +{
>>> +    int ret = -1;
>>> +
>>> +    switch (chr->deviceType) {
>>> +    case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL:
>>> +        switch ((virDomainChrSerialTargetType) chr->targetType) {
>>> +        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
>>> +        case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB:
>>> +            ret = 0;
>>> +            break;
>>> +
>>> +        default:
>> Typically in switches listing other options rather than default:

The point of casting it to virDomainChrSerialTargetType is to catch all the
unhandled values and it only works if there's no default: clause.

Also I don't think we need the ret variable at all. Just return 0 at the end
of the function, or -1 if we reached an unsupported combination.

>>
>>
>> I think perhaps this one is better than 1/2, but will see if my
>> responses result in other opinions...

Even if this one fixes the bug, 1/2 is nice to have.

> 
> Thanks for pointing out and i forgot cc Jan and Pavel when sent this patch,
> maybe they have some other opinions.
>

No need to cc me, I am subscribed to the list. :)

Jan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150121/e548dd0f/attachment-0001.sig>