1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v2 00/15] qemu: Validate PCI controller options
  4. View patch

[libvirt] [PATCH v2 05/15] qemu: Simplify modelName stringification

Andrea Bolognani posted 15 patches 7 years, 2 months ago
There is a newer version of this series
[libvirt] [PATCH v2 05/15] qemu: Simplify modelName stringification
Posted by Andrea Bolognani 7 years, 2 months ago 
There's no need to perform checks before conversion, we can
just go try and check the results later on.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_command.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2291bf5da..a44a1b2d2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2725,13 +2725,8 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
         break;
 
     case VIR_DOMAIN_CONTROLLER_TYPE_PCI: {
-        const virDomainPCIControllerOpts *pciopts;
-        const char *modelName = NULL;
-
-        pciopts = &def->opts.pciopts;
-        if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
-            def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST)
-            modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
+        const virDomainPCIControllerOpts *pciopts = &def->opts.pciopts;
+        const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
 
         /* Skip the implicit PHB for pSeries guests */
         if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
@@ -2741,6 +2736,13 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
             goto done;
         }
 
+        if (!modelName) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Unknown virDomainControllerPCIModelName value: %d"),
+                           pciopts->modelName);
+            return -1;
+        }
+
         switch ((virDomainControllerModelPCI) def->model) {
         case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
             virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s",
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 05/15] qemu: Simplify modelName stringification
Posted by Peter Krempa 7 years, 2 months ago 
On Fri, Feb 16, 2018 at 17:28:02 +0100, Andrea Bolognani wrote:
> There's no need to perform checks before conversion, we can
> just go try and check the results later on.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  src/qemu/qemu_command.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 2291bf5da..a44a1b2d2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2725,13 +2725,8 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
>          break;
>  
>      case VIR_DOMAIN_CONTROLLER_TYPE_PCI: {
> -        const virDomainPCIControllerOpts *pciopts;
> -        const char *modelName = NULL;
> -
> -        pciopts = &def->opts.pciopts;
> -        if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
> -            def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST)
> -            modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> +        const virDomainPCIControllerOpts *pciopts = &def->opts.pciopts;
> +        const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);

This is not equivalent. virDomainControllerPCIModelName implementation
actually defines a string for VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT so
it will not return NULL and the commit message does not explain why/if
it is actually okay.

>  
>          /* Skip the implicit PHB for pSeries guests */
>          if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT &&
> @@ -2741,6 +2736,13 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
>              goto done;
>          }
>  
> +        if (!modelName) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Unknown virDomainControllerPCIModelName value: %d"),

I think this was under discussion somewhere in this series.

> +                           pciopts->modelName);
> +            return -1;
> +        }
> +
>          switch ((virDomainControllerModelPCI) def->model) {
>          case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>              virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s",
> -- 
> 2.14.3
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 05/15] qemu: Simplify modelName stringification
Posted by Andrea Bolognani 7 years, 2 months ago 
On Mon, 2018-02-19 at 16:06 +0100, Peter Krempa wrote:
> >      case VIR_DOMAIN_CONTROLLER_TYPE_PCI: {
> > -        const virDomainPCIControllerOpts *pciopts;
> > -        const char *modelName = NULL;
> > -
> > -        pciopts = &def->opts.pciopts;
> > -        if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
> > -            def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST)
> > -            modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> > +        const virDomainPCIControllerOpts *pciopts = &def->opts.pciopts;
> > +        const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> 
> This is not equivalent. virDomainControllerPCIModelName implementation
> actually defines a string for VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT so
> it will not return NULL and the commit message does not explain why/if
> it is actually okay.

The result doesn't change because the PCIE_ROOT case in the switch
statement immediately following the conversion does nothing but
error out, which makes whether or not modelName is NULL irrelevant.

I can add the explanation above to the commit message if you want.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 05/15] qemu: Simplify modelName stringification
Posted by Peter Krempa 7 years, 2 months ago 
On Mon, Feb 19, 2018 at 16:26:35 +0100, Andrea Bolognani wrote:
> On Mon, 2018-02-19 at 16:06 +0100, Peter Krempa wrote:
> > >      case VIR_DOMAIN_CONTROLLER_TYPE_PCI: {
> > > -        const virDomainPCIControllerOpts *pciopts;
> > > -        const char *modelName = NULL;
> > > -
> > > -        pciopts = &def->opts.pciopts;
> > > -        if (def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
> > > -            def->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST)
> > > -            modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> > > +        const virDomainPCIControllerOpts *pciopts = &def->opts.pciopts;
> > > +        const char *modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> > 
> > This is not equivalent. virDomainControllerPCIModelName implementation
> > actually defines a string for VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT so
> > it will not return NULL and the commit message does not explain why/if
> > it is actually okay.
> 
> The result doesn't change because the PCIE_ROOT case in the switch
> statement immediately following the conversion does nothing but
> error out, which makes whether or not modelName is NULL irrelevant.

I mistakenly looked into the case for PCI_ROOT, so that mislead me. I
think it's okay in this case even without a comment. Thanks for the
explanation.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.