Re: [libvirt] [PATCH v2 08/15] qemu: Validate PCI controllers (index)

2018-02-19 Thread Andrea Bolognani 
On Mon, 2018-02-19 at 11:52 +, Daniel P. Berrangé wrote:
> > +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> > *controller,
> > + const virDomainDef *def)
> >  {
> > +const virDomainPCIControllerOpts *pciopts = &controller->opts.pciopts;
> > +const char *model = 
> > virDomainControllerModelPCITypeToString(controller->model);
> > +const char *modelName = 
> > virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> > +
> > +if (!model) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Unknown virDomainControllerModelPCI value: %d"),
> 
> Including type names in error messages is not too nice as a user facing
> error - better to use plain english  "Unexpected PCI controller model")

But these are marked as internal errors, eg. They Should Never
Happen™, and if they ever do it's useful for developers to have
more detailed information. Users can't really do anything but
report the issue, after all.

> > +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> > +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> > +/* pci-root controllers for pSeries guests can have any index, but
> > + * all other pci-root and pcie-root controller must have index 0 */
> > +if (controller->idx != 0 &&
> > +!(qemuDomainIsPSeries(def) &&
> > +  controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)) {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +   _("Index for '%s' controller must be 0"),
> > +   model);
> > +return -1;
> > +}
> > +break;
> > +
> > +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> 
> Any time there is a _LAST element we should have an error report too
> 
>virReportError(VIR_ERR_INTERNAL_ERROR,
>   _("Unexpected controller model %d"), controller->model);
>return -1;
> 
> It should also have an 'default:' next to the LAST. Both are things that
> should never occur unless we've screwed up code elsewherein libvirt, but
> we want to be robust about catching such screw ups. This is particularly
> important for controller models, as we have many different controller
> type specific enums all stored in the same struct field

Okay, I'll address that, but the point above about what error
message to show in such scenarios still applies IMHO.

-- 
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 08/15] qemu: Validate PCI controllers (index)

2018-02-19 Thread Daniel P . Berrangé 
On Fri, Feb 16, 2018 at 05:28:05PM +0100, Andrea Bolognani wrote:
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_domain.c | 56 
> --
>  1 file changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 9dc3d5597..e4e67c585 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4243,9 +4243,61 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
> virDomainControllerDef *controll
>  
>  
>  static int
> -qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> *controller ATTRIBUTE_UNUSED,
> - const virDomainDef *def 
> ATTRIBUTE_UNUSED)
> +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> *controller,
> + const virDomainDef *def)
>  {
> +const virDomainPCIControllerOpts *pciopts = &controller->opts.pciopts;
> +const char *model = 
> virDomainControllerModelPCITypeToString(controller->model);
> +const char *modelName = 
> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> +
> +if (!model) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown virDomainControllerModelPCI value: %d"),

Including type names in error messages is not too nice as a user facing
error - better to use plain english  "Unexpected PCI controller model")

> +   controller->model);
> +return -1;
> +}
> +if (!modelName) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown virDomainControllerPCIModelName value: 
> %d"),
> +   pciopts->modelName);

Likewise here.

> +return -1;
> +}
> +
> +/* index */
> +switch ((virDomainControllerModelPCI) controller->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
> +if (controller->idx == 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Index for '%s' controller must be > 0"),
> +   model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +/* pci-root controllers for pSeries guests can have any index, but
> + * all other pci-root and pcie-root controller must have index 0 */
> +if (controller->idx != 0 &&
> +!(qemuDomainIsPSeries(def) &&
> +  controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Index for '%s' controller must be 0"),
> +   model);
> +return -1;
> +}
> +break;
> +
> +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:

Any time there is a _LAST element we should have an error report too

   virReportError(VIR_ERR_INTERNAL_ERROR,
  _("Unexpected controller model %d"), controller->model);
   return -1; 

It should also have an 'default:' next to the LAST. Both are things that
should never occur unless we've screwed up code elsewherein libvirt, but
we want to be robust about catching such screw ups. This is particularly
important for controller models, as we have many different controller
type specific enums all stored in the same struct field

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 08/15] qemu: Validate PCI controllers (index)

2018-02-16 Thread Andrea Bolognani 
Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_domain.c | 56 --
 1 file changed, 54 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9dc3d5597..e4e67c585 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4243,9 +4243,61 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
virDomainControllerDef *controll
 
 
 static int
-qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
*controller ATTRIBUTE_UNUSED,
- const virDomainDef *def 
ATTRIBUTE_UNUSED)
+qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
*controller,
+ const virDomainDef *def)
 {
+const virDomainPCIControllerOpts *pciopts = &controller->opts.pciopts;
+const char *model = 
virDomainControllerModelPCITypeToString(controller->model);
+const char *modelName = 
virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
+
+if (!model) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unknown virDomainControllerModelPCI value: %d"),
+   controller->model);
+return -1;
+}
+if (!modelName) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unknown virDomainControllerPCIModelName value: %d"),
+   pciopts->modelName);
+return -1;
+}
+
+/* index */
+switch ((virDomainControllerModelPCI) controller->model) {
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
+if (controller->idx == 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Index for '%s' controller must be > 0"),
+   model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+/* pci-root controllers for pSeries guests can have any index, but
+ * all other pci-root and pcie-root controller must have index 0 */
+if (controller->idx != 0 &&
+!(qemuDomainIsPSeries(def) &&
+  controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Index for '%s' controller must be 0"),
+   model);
+return -1;
+}
+break;
+
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+break;
+}
+
 return 0;
 }
 
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list