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

[Luyao Huang]

On 01/21/2015 08:00 PM, Ján Tomko wrote:
> 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.
>

Thanks for pointing out, error output more earlier more better :)
>>>> 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.

Good idea ! i will remove the ret and use return in these place.
>>>
>>> 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. :)

I just saw other people cc the people who help review the patch when 
send a new version
patch to upstream, so i think it is better to cc the people who help 
review the patch when write
a new version.

Thanks a lot for your review.
>
> Jan
>
>

Luyao