Re: [PATCH v3 08/12] qemu: Move PCI command modelName check to controller def validate

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux