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

[Laine Stump]

On 05/08/2015 01:42 PM, John Ferlan wrote:
>
> 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,

Okay, right. Not sure why I put it in that order.

>
> 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.

Correct. I was saying that removing the catchall bit wouldn't work
because it would be needed when it was IDE/SATA but not primary.

Anyway, I've decided that I agree with your later asessment that, since
this is for exceptions, I should just retain the if .... else if ...
else structure of the original rather than change it to a switch. And
since I'm doing that, I'm merging the addition of argument(s) (turns out
I need qemuCaps in addition to DomainDef) into a single patch that adds
the exceptions for primary sata and ide controllers.