[libvirt] [PATCH 04/13] qemu: restructure qemuAssignDeviceControllerAlias

[John Ferlan]

On 05/08/2015 01:17 PM, Laine Stump wrote:
> On 05/08/2015 12:51 PM, John Ferlan wrote:
>>
>> On 05/05/2015 02:03 PM, Laine Stump wrote:
>>> No functional change, just rearrange this function and pass in full
>>> domain definition to make it simpler to add exceptions.
>>> ---
>>>  src/qemu/qemu_command.c | 32 ++++++++++++++++++++++----------
>>>  src/qemu/qemu_command.h |  2 +-
>>>  src/qemu/qemu_hotplug.c |  4 ++--
>>>  3 files changed, 25 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index b76bd98..340478c 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -1031,22 +1031,34 @@ qemuAssignDeviceRedirdevAlias(virDomainDefPtr def, virDomainRedirdevDefPtr redir
>>>  
>>>  
>>>  int
>>> -qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
>>> +qemuAssignDeviceControllerAlias(ATTRIBUTE_UNUSED virDomainDefPtr def,
>> ?? Not using it now or in this series to add exceptions, so is there
>> really a need for it?
> 
> Patch 6 - add exceptions for primate ide/sata controllers.
> 

oh - right... but I was wondering why no compiler complaint on the
ATTRIBUTE_UNUSED... Perhaps it's "ordering related"...

That is should it be:

virDomainDefPtr def ATTRIBUTE_UNUSED,

here... then in patch 6 it gets removed...

That's why my eyes locked onto...


>>
>>> +                                virDomainControllerDefPtr controller)
>>>  {
>>> -    const char *prefix = virDomainControllerTypeToString(controller->type);
>>> +    int ret = 0;
>>> +
>>> +    VIR_FREE(controller->info.alias);
>>>  
>>> -    if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
>>> -        /* only pcie-root uses a different naming convention
>>> -         * ("pcie.0"), because it is hardcoded that way in qemu. All
>>> -         * other buses use the consistent "pci.%u".
>>> +    switch (controller->type) {
>> Similar to 3/13
>>
>> s/(/((virDomainControllerType)/
>>
>>> +    case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
>>> +        /* pcie-root uses a different naming convention ("pcie.0"),
>>> +         * because it is hardcoded that way in qemu. All other PCI
>>> +         * buses use the consistent "pci.%u".
>>>           */
>>>          if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
>>> -            return virAsprintf(&controller->info.alias, "pcie.%d", controller->idx);
>>> +            ret = virAsprintf(&controller->info.alias, "pcie.%d", controller->idx);
>>>          else
>>> -            return virAsprintf(&controller->info.alias, "pci.%d", controller->idx);
>>> +            ret = virAsprintf(&controller->info.alias, "pci.%d", controller->idx);
>>> +        break;
>>> +    default:
>> Change default to:
>>
>>     case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
>>     case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
>>     case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
>>     case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
>>     case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
>>     case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
>>     case VIR_DOMAIN_CONTROLLER_TYPE_USB:
>>         ret = virAsprintf(&controller->info.alias, "%s%d",
>>                           virDomainControllerTypeToString(controller->type),
>>                           controller->idx);
>>         break;
>>
>>     case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
>>         Some sort of virReportError...
>>         ret = -1;
>>> +        break;
>>>      }
>>>  
>>> -    return virAsprintf(&controller->info.alias, "%s%d", prefix, controller->idx);
>>> +    /* catchall for anything that wasn't an exception */
>>> +    if (ret == 0 && !controller->info.alias)
>>> +        ret = virAsprintf(&controller->info.alias, "%s%d",
>>> +                          virDomainControllerTypeToString(controller->type),
>>> +                          controller->idx);
>> Thus removing the need for this ^^^^
> 
> That won't work, because the cases for IDE and SATA don't always set the
> alias (they only set it or machinetypes and controller indexes where it
> needs to be an exception).
> 

Huh?   Patch 6 has those exceptions, but the way I'm reading how you
wrote the code an alias would be assigned (either "ide.%d" or "sata.%d")
because you'd fall into the catchall code as ret would be 0 and
info.alias would be NULL.

>> ACK w/ adjustments... Although I'm still unsure why we want to pass
>> the whole domain def...
> 
> You need the DomainDefPtr so you check the machinetype with
> qemuDomainMachineIs*()
> 
> 

Yes, now I see - again, locked onto the ATTRIBUTE_UNUSED

John