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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list