10:10 a.m.
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(a)redhat.com>
> ---
> 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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list