Re: [libvirt] [PATCH 2/2] qemu: add a check when cold-plug a Chr device
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(a)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
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)