[libvirt] [PATCH 06/13] qemu: add exceptions for alias names of primary sata/ide controllers

[John Ferlan]

On 05/08/2015 02:12 PM, Laine Stump wrote:
> On 05/08/2015 01:47 PM, John Ferlan wrote:
>>
>> On 05/05/2015 02:03 PM, Laine Stump wrote:
>>> If a machine is Q35, the primary sata controller is hardcoded in qemu
>>> to have the id "ide" (with no index in the name). Likewise on
>>> 440fx-based machinetypes, the primary ide controller is called
>>> "ide". All other SATA controllers will have the standard name
>>> "sata%d", where %d is the index of the controller, and if any
>>> additional IDE controllers are ever supported, they will have the name
>>> "ide%d".
>>> ---
>>>  src/qemu/qemu_command.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 340478c..89beeb3 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -1049,6 +1049,21 @@ qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED virDomainDefPtr def,
>>>          else
>>>              ret = virAsprintf(&controller->info.alias, "pci.%d", controller->idx);
>>>          break;
>>> +    case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
>>> +        /* for any machine based on I440FX, the first (currently only)
>>> +         * IDE controller is an integrated controller hardcoded with
>>> +         * id "ide"
>>> +         */
>>> +        if (qemuDomainMachineIsI440FX(def) && controller->idx == 0)
>>> +            ret = VIR_STRDUP(controller->info.alias, "ide");
>> Based on my review of 4/13 and my followup comment, there'd need to be
>> an "else" here with does the :
>>
>>          ret = virAsprintf(&controller->info.alias, "%s%d",
>>                            virDomainControllerTypeToString(controller->type),
>>                            controller->idx);
> 
> Right, that's why I put that extra "catchall" at the bottom of the
> function. I wanted to avoid duplicating code.
> 
>>
>>
>>> +        break;
>>> +    case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
>>> +        /* for any Q35 machine, the first SATA controller is the
>>> +         * integrated one, and it too is hardcoded with id "ide"
>>> +         */
>>> +        if (qemuDomainMachineIsQ35(def) && controller->idx == 0)
>>> +            ret = VIR_STRDUP(controller->info.alias, "ide");
>> similarly, else
>>          ret = virAsprintf(&controller->info.alias, "%s%d",
>>                            virDomainControllerTypeToString(controller->type),
>>                            controller->idx);
> 
> ... like this :-)
> 

Right - not that I want to rewrite the code; however, I think the
purpose of using a switch is to catch all the options, especially with
the typecasting of the switch... While perhaps an if ... else if ...
would be more suitable for the exceptions such as this.

Maybe someone else has a strong enough opinion to warrant a fully
populated switch.

What you have works, but may not fit everyone's style/taste.

I'm OK with it though

John