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&gt; >>> --- >>> 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