On 10.07.2013 15:18, Daniel P. Berrange wrote: > 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 > Now that I am re-thinking this over, it's not a bug anymore. Libvirt already supports multiple consoles, and not all of them are just an alias to a serial console. That means, def->consoles[i]->info can be either "serialX" or "consoleX" regardless of my fix. This means, I have to deal with both aliases in this patch. But okay, I'll leave out the 'if (idx == -1) {}', and add it in later patch when needed. As a separate function. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list