Re: [libvirt] [PATCH v3 6/9] qemu_command: Honour chardev alias assignment with a function

On Wed, Jul 10, 2013 at 02:57:48PM +0200, Michal Privoznik wrote: > On 03.07.2013 17:29, Daniel P. Berrange wrote: >> On Tue, Jul 02, 2013 at 05:53:05PM +0200, Michal Privoznik wrote: >>> The chardev alias assignment is going to be needed in a separate >>> places, so it should be moved into a separate function rather >>> than copying code randomly around. >>> --- >>> src/qemu/qemu_command.c | 75 +++++++++++++++++++++++++++++++++++++++++++------ >>> src/qemu/qemu_command.h | 3 ++ >>> 2 files changed, 70 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >>> index ba93233..903839f 100644 >>> --- a/src/qemu/qemu_command.c >>> +++ b/src/qemu/qemu_command.c >>> @@ -892,6 +892,65 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) >>> return 0; >>> } >>> >>> +int >>> +qemuAssignDeviceChrAlias(virDomainDefPtr def, >>> + virDomainChrDefPtr chr, >>> + ssize_t idx) >>> +{ >>> + const char *prefix = NULL; >>> + const char *prefix2 = NULL; >>> + >>> + switch ((enum virDomainChrDeviceType) chr->deviceType) { >>> + case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: >>> + prefix = "parallel"; >>> + break; >>> + >>> + case VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL: >>> + prefix = "serial"; >>> + break; >>> + >>> + case VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE: >>> + prefix = "console"; >>> + prefix2 = "serial"; >>> + break; >>> + >>> + case VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL: >>> + prefix = "channel"; >>> + break; >>> + >>> + case VIR_DOMAIN_CHR_DEVICE_TYPE_LAST: >>> + return -1; >>> + } >>> + >>> + if (idx == -1) { >>> + virDomainChrDefPtr **arrPtr; >>> + size_t *cntPtr; >>> + size_t i; >>> + idx = 0; >>> + >>> + virDomainChrGetDomainPtrs(def, chr, &arrPtr, &cntPtr); >>> + >>> + for (i = 0; i < *cntPtr; i++) { >>> + int thisidx; >>> + if (((thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix)) < 0) && >>> + (prefix2 && >>> + (thisidx = qemuDomainDeviceAliasIndex(&(*arrPtr)[i]->info, prefix2)) < 0)) { >>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >>> + _("Unable to determine device index for character device")); >>> + return -1; >>> + } >>> + if (thisidx >= idx) >>> + idx = thisidx + 1; >>> + } >>> + } >> >> The commit message describes this as a simple refactoring, but this >> if (idx== -1) {...} is all new functionality compared to what is >> being replaced. I'm not too sure that this logic is correct >> either when dealing with <console> with a 'serialXX' alias. > > This function just imitates other functions we've already: > qemuAssignDeviceRedirdevAlias, qemuAssignDeviceHostdevAlias being > examples. One can find even more. Sure, that's not what I was complaining about though. The issue is that you're mixing plain refactoring of code, with the inclusion of extra functionality. Nothing in this patch ever passes a value 'idx == -1' so code to handle that scenario is not related to refactoring. It can be introduced in whatever patch actually needs that code. > And regarding <console> I've sent 2 patches, none of them was accepted. > So I had to workaround the problem in my patch. What's the solution > you're suggesting? Just to refresh our memory: for <console> the alias > can be either 'consoleX' or 'serialX' depending if the daemon was > restarted or not. I described what I think needs fixing here: https://www.redhat.com/archives/libvir-list/2013-July/msg00107.html > IIUC, this patch is intended to change things so that after libvirtd is > restarted, we get: > > def->seriales[0]->info == "serial0" > def->consoles[0]->info == "console0" > > but this is fixing the wrong thing. There is only one physical device > emulated in the guest, which is a serial port with id==serial0, and > this is reflected correctly in the XML we generate. Only the internal > struct is different. > > So what needs fixing is the code which populated def->consoles[0]->info > with "console0" instead of the correct "serial0" string at VM startup. Daniel