On 12/08/2017 10:48 AM, Ján Tomko wrote:
> On Wed, Dec 06, 2017 at 08:14:04PM -0500, John Ferlan wrote:
>> Move the various modelName == NAME_NONE from the command line
>> generation into domain controller validation. Also rather than
>> have multiple cases with the same check, let's make the code
>> more generic, but also note that it was the modelName option
>> that caused the failure. We also have to be sure not to check
>> the PCI models that we don't care about.
>>
>> For the remaining checks in command line building, we can use
>> the field name in the error message to be more specific about
>> what causes the failure.
>
> The errors should all be unreachable. I don't see a need to make them
> more specific and bother the translators with changes in them.
>
Hmmm... I see... Although virDomainControllerDefParseXML does only parse
them if they exist, the generation of <model name='%s'/> is "hidden", so
splatting the error for what should never happen doesn't seem like a
good idea. Similarly for the other options as well. So I'll make them
all the same generic error message (even though it does seem a bit
strange personally).
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/qemu/qemu_command.c | 52
>> ++++++++++++-------------------------------------
>> src/qemu/qemu_domain.c | 12 ++++++++++++
>> 2 files changed, 24 insertions(+), 40 deletions(-)
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 4e292e446..45cbf5381 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -2722,11 +2722,9 @@ qemuBuildControllerDevStr(const virDomainDef
>> *domainDef,
>>
>> switch ((virDomainControllerModelPCI) def->model) {
>> case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>> - if (pciopts->modelName
>> - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE ||
>> - pciopts->chassisNr == -1) {
>> + if (pciopts->chassisNr == -1) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("autogenerated pci-bridge options
>> not set"));
>> + _("autogenerated pci-bridge option
>> chassisNr not set"));
>> goto error;
>> }
>>
>> @@ -2756,12 +2754,9 @@ qemuBuildControllerDevStr(const virDomainDef
>> *domainDef,
>> def->info.alias);
>> break;
>> case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
>> - if (pciopts->modelName
>> - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE ||
>> - pciopts->busNr == -1) {
>> + if (pciopts->busNr == -1) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("autogenerated pci-expander-bus
>> options not set"));
>> - goto error;
>> + _("autogenerated pci-expander-bus
>> option busNr not set"));
>> }
>>
>> modelName =
>> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
>> @@ -2793,13 +2788,6 @@ qemuBuildControllerDevStr(const virDomainDef
>> *domainDef,
>> pciopts->numaNode);
>> break;
>> case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
>> - if (pciopts->modelName
>> - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("autogenerated dmi-to-pci-bridge
>> options not set"));
>> - goto error;
>> - }
>> -
>> modelName =
>> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
>> if (!modelName) {
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -2824,12 +2812,6 @@ qemuBuildControllerDevStr(const virDomainDef
>> *domainDef,
>> virBufferAsprintf(&buf, "%s,id=%s", modelName,
>> def->info.alias);
>> break;
>> case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
>> - if (pciopts->modelName
>> - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("autogenerated pcie-root-port
>> options not set"));
>> - goto error;
>> - }
>> modelName =
>> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
>> if (!modelName) {
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -2869,12 +2851,6 @@ qemuBuildControllerDevStr(const virDomainDef
>> *domainDef,
>> pciopts->chassis, def->info.alias);
>> break;
>> case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
>> - if (pciopts->modelName
>> - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("autogenerated
>> pcie-switch-upstream-port options not set"));
>> - goto error;
>> - }
>> modelName =
>> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
>> if (!modelName) {
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> @@ -2900,13 +2876,12 @@ qemuBuildControllerDevStr(const virDomainDef
>> *domainDef,
>> virBufferAsprintf(&buf, "%s,id=%s", modelName,
>> def->info.alias);
>> break;
>> case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
>> - if (pciopts->modelName
>> - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE ||
>> - pciopts->chassis == -1 ||
>> + if (pciopts->chassis == -1 ||
>> pciopts->port == -1) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("autogenerated
>> pcie-switch-downstream-port "
>> - "options not set"));
>> + "options chassis=%d or port=%d not
>> set"),
>> + pciopts->chassis, pciopts->port);
>> goto error;
>> }
>>
>> @@ -2937,11 +2912,9 @@ qemuBuildControllerDevStr(const virDomainDef
>> *domainDef,
>> pciopts->chassis, def->info.alias);
>> break;
>> case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
>> - if (pciopts->modelName
>> - == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE ||
>> - pciopts->busNr == -1) {
>> + if (pciopts->busNr == -1) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("autogenerated pcie-expander-bus
>> options not set"));
>> + _("autogenerated pcie-expander-bus
>> option busNr not set"));
>> goto error;
>> }
>>
>> @@ -2974,10 +2947,9 @@ qemuBuildControllerDevStr(const virDomainDef
>> *domainDef,
>> pciopts->numaNode);
>> break;
>> case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
>> - if (pciopts->modelName ==
>> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE ||
>> - pciopts->targetIndex == -1) {
>> + if (pciopts->targetIndex == -1) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> - _("autogenerated pci-root options not
>> set"));
>> + _("autogenerated pci-root option
>> targetIndex not set"));
>> goto error;
>> }
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index ceb03a0cd..ecfff4209 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -4014,6 +4014,7 @@ qemuDomainDeviceDefValidateControllerPCI(const
>> virDomainControllerDef *controlle
>> const virDomainDef *def)
>> {
>> virDomainControllerModelPCI model = controller->model;
>> + const virDomainPCIControllerOpts *pciopts;
>>
>> /* skip pcie-root */
>> if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> @@ -4049,6 +4050,17 @@ qemuDomainDeviceDefValidateControllerPCI(const
>> virDomainControllerDef *controlle
>> break;
>> }
>>
>> + pciopts = &controller->opts.pciopts;
>> + if (controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
>> + controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) {
>> + if (pciopts->modelName ==
>> VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("autogenerated %s option modelName not
>> set"),
>
> %s not surrounded by any quotes. IMO 'autogenerated pci controller
> %option not set' should be enough for all these "errors"
>
In this case the "autogenerated %s" wasn't single quoted before. I
could single quote it, but that makes it "different" than before. I did
check that all the %s names matched the virDomainControllerModelPCI list
of names. I think it should stay without the single quote.
John
> Jan
>
>> +
>> virDomainControllerModelPCITypeToString(controller->model));
>> + return -1;
>> + }
>> + }
>> +
>> return 0;
>> }
>>
>> --
>> 2.13.6
>>
>> --
>> libvir-list mailing list
>> libvir-list@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
