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

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.

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

> 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*()