Re: [libvirt] [PATCH v3 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Laine Stump 
On 03/02/2018 08:05 AM, Andrea Bolognani wrote:
> On Fri, 2018-03-02 at 07:34 -0500, Laine Stump wrote:
>> On 02/21/2018 09:14 AM, Andrea Bolognani wrote:
> [...]
>>> +static int
>>> +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
>>> *cont,
>>> + const virDomainDef *def,
>>> + virQEMUCapsPtr qemuCaps)
>>> +
>>> +{
>>> +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
>>> +const char *model = 
>>> virDomainControllerModelPCITypeToString(cont->model);
>>> +const char *modelName = 
>>> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
>>> +
>>> +if (!model) {
>>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +   _("Unknown virDomainControllerModelPCI value: %d"),
>>> +   cont->model);
>>> +return -1;
>>> +}
>>> +if (!modelName) {
>>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +   _("Unknown virDomainControllerPCIModelName value: 
>>> %d"),
>>> +   pciopts->modelName);
>>> +return -1;
>>> +}
>> (meant to send this before, but kept forgetting...)
>>
>> 1) I thought modelName wasn't set for pci-root. Doesn't the above cause
>> a validation error in that case? (too early in the day, haven't tried it)
> The default value is _MODEL_NAME_NONE aka zero, which is still part
> of the enumeration, so virDomainControllerPCIModelNameTypeToString()
> won't return NULL and no error will be raised. For pSeries guests,
> it will be set to _MODEL_NAME_SPAPR_PCI_HOST_BRIDGE so once again no
> problem there.

Ah, right. So the name is "none", but when we're formatting for dumpxml
we skip it if the value is 0, so that never shows up. "Nevermind"


>
>> 2) danpb made a nice new function to standardize/simplify errors of the
>> above type: virReportEnumRangeError().
> His efforts on switch normalization and me rebasing this series
> happened pretty much at the same time; more specifically, the
> function you're talking about was introduced in 3b1020ac805e, while
> my series is based on the earlier f565321b26df.

Yep, I saw his patches at about the same time as yours, and since you're
respinning I thought I'd point it out.

> I guess this means another rebase! Yay! \o/

Someday you'll learn to do what I'm doing right now - I want to add some
extra validation of pci controller options, but I'm waiting to write any
code until you've pushed your patches. :-)

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

Re: [libvirt] [PATCH v3 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Andrea Bolognani 
On Fri, 2018-03-02 at 07:34 -0500, Laine Stump wrote:
> On 02/21/2018 09:14 AM, Andrea Bolognani wrote:
[...]
> > +static int
> > +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> > *cont,
> > + const virDomainDef *def,
> > + virQEMUCapsPtr qemuCaps)
> > +
> > +{
> > +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
> > +const char *model = 
> > virDomainControllerModelPCITypeToString(cont->model);
> > +const char *modelName = 
> > virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> > +
> > +if (!model) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Unknown virDomainControllerModelPCI value: %d"),
> > +   cont->model);
> > +return -1;
> > +}
> > +if (!modelName) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR,
> > +   _("Unknown virDomainControllerPCIModelName value: 
> > %d"),
> > +   pciopts->modelName);
> > +return -1;
> > +}
> 
> (meant to send this before, but kept forgetting...)
> 
> 1) I thought modelName wasn't set for pci-root. Doesn't the above cause
> a validation error in that case? (too early in the day, haven't tried it)

The default value is _MODEL_NAME_NONE aka zero, which is still part
of the enumeration, so virDomainControllerPCIModelNameTypeToString()
won't return NULL and no error will be raised. For pSeries guests,
it will be set to _MODEL_NAME_SPAPR_PCI_HOST_BRIDGE so once again no
problem there.

> 2) danpb made a nice new function to standardize/simplify errors of the
> above type: virReportEnumRangeError().

His efforts on switch normalization and me rebasing this series
happened pretty much at the same time; more specifically, the
function you're talking about was introduced in 3b1020ac805e, while
my series is based on the earlier f565321b26df.

I guess this means another rebase! Yay! \o/

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

2018-03-02 Thread Laine Stump 
On 02/21/2018 09:14 AM, Andrea Bolognani wrote:
> The esisting function is renamed and called from the new one, so
> that even while we're in the process of implementing new checks
> all the existing ones will be performed.
>
> Signed-off-by: Andrea Bolognani 
> ---
> Based on the reviewer's preference, this patch can be pushed on
> its own or squashed into the next one.
>
>  src/qemu/qemu_domain.c | 33 ++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b1308e5a49..151d33aee3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4247,9 +4247,9 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
> virDomainControllerDef *controll
>  
>  
>  static int
> -qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
> *controller,
> - const virDomainDef *def,
> - virQEMUCapsPtr qemuCaps)
> +qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef 
> *controller,
> +const virDomainDef *def,
> +virQEMUCapsPtr qemuCaps)
>  {
>  virDomainControllerModelPCI model = controller->model;
>  const virDomainPCIControllerOpts *pciopts;
> @@ -4526,6 +4526,33 @@ qemuDomainDeviceDefValidateControllerPCI(const 
> virDomainControllerDef *controlle
>  }
>  
>  
> +static int
> +qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont,
> + const virDomainDef *def,
> + virQEMUCapsPtr qemuCaps)
> +
> +{
> +const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
> +const char *model = virDomainControllerModelPCITypeToString(cont->model);
> +const char *modelName = 
> virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
> +
> +if (!model) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown virDomainControllerModelPCI value: %d"),
> +   cont->model);
> +return -1;
> +}
> +if (!modelName) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Unknown virDomainControllerPCIModelName value: 
> %d"),
> +   pciopts->modelName);
> +return -1;
> +}

(meant to send this before, but kept forgetting...)

1) I thought modelName wasn't set for pci-root. Doesn't the above cause
a validation error in that case? (too early in the day, haven't tried it)

2) danpb made a nice new function to standardize/simplify errors of the
above type: virReportEnumRangeError().


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

[libvirt] [PATCH v3 02/12] qemu: Create new qemuDomainDeviceDefValidateControllerPCI()

2018-02-21 Thread Andrea Bolognani 
The esisting function is renamed and called from the new one, so
that even while we're in the process of implementing new checks
all the existing ones will be performed.

Signed-off-by: Andrea Bolognani 
---
Based on the reviewer's preference, this patch can be pushed on
its own or squashed into the next one.

 src/qemu/qemu_domain.c | 33 ++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b1308e5a49..151d33aee3 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4247,9 +4247,9 @@ qemuDomainDeviceDefValidateControllerSCSI(const 
virDomainControllerDef *controll
 
 
 static int
-qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef 
*controller,
- const virDomainDef *def,
- virQEMUCapsPtr qemuCaps)
+qemuDomainDeviceDefValidateControllerPCIOld(const virDomainControllerDef 
*controller,
+const virDomainDef *def,
+virQEMUCapsPtr qemuCaps)
 {
 virDomainControllerModelPCI model = controller->model;
 const virDomainPCIControllerOpts *pciopts;
@@ -4526,6 +4526,33 @@ qemuDomainDeviceDefValidateControllerPCI(const 
virDomainControllerDef *controlle
 }
 
 
+static int
+qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *cont,
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
+
+{
+const virDomainPCIControllerOpts *pciopts = >opts.pciopts;
+const char *model = virDomainControllerModelPCITypeToString(cont->model);
+const char *modelName = 
virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
+
+if (!model) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unknown virDomainControllerModelPCI value: %d"),
+   cont->model);
+return -1;
+}
+if (!modelName) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unknown virDomainControllerPCIModelName value: %d"),
+   pciopts->modelName);
+return -1;
+}
+
+return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
+}
+
+
 static int
 qemuDomainDeviceDefValidateControllerSATA(const virDomainControllerDef 
*controller,
   const virDomainDef *def,
-- 
2.14.3

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