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

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.

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.

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.