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

[Michal Privoznik]

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