4:42 p.m.
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.