[v2] move checks from parse to validate callbacks

[PATCH v2 0/9] move checks from parse to validate callbacks

Posted by Daniel Henrique Barboza 3 years, 4 months ago

Hi,

This is the respin of [1] after the reviews from Michal.
Although we're pushing code to validate callbacks instead of
post parse functions, the idea and motivation is still in line
with [2]. 


[1] https://www.redhat.com/archives/libvir-list/2020-November/msg01409.html
[2] https://gitlab.com/libvirt/libvirt/-/issues/7


Daniel Henrique Barboza (9):
  domain_conf.c: move boot related timeouts check to validate callback
  domain_conf.c: move primary video check to validate callback
  domain_conf.c: move QXL attributes check to
    virDomainVideoDefValidate()
  domain_conf.c: move vendor, product and tray checks to validate
    callback
  domain_conf.c: move smartcard address check to validate callback
  domain_conf.c: move blkio path check to validate callback
  domain_conf.c: move virDomainPCIControllerOpts checks to validate
    callback
  domain_conf.c: move pci-root/pcie-root address check to validateCB
  domain_conf.c: move idmapEntry checks to validate callback

 src/conf/domain_conf.c                        | 353 +++++++++++-------
 tests/qemuxml2argvdata/pci-root-address.err   |   2 +-
 .../pseries-default-phb-numa-node.err         |   2 +-
 .../video-multiple-primaries.err              |   1 +
 .../video-multiple-primaries.xml              |  32 ++
 tests/qemuxml2argvtest.c                      |  14 +-
 6 files changed, 268 insertions(+), 136 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err
 create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml

-- 
2.26.2

Re: [PATCH v2 0/9] move checks from parse to validate callbacks

Posted by Michal Privoznik 3 years, 4 months ago

On 12/7/20 2:54 PM, Daniel Henrique Barboza wrote:
> Hi,
> 
> This is the respin of [1] after the reviews from Michal.
> Although we're pushing code to validate callbacks instead of
> post parse functions, the idea and motivation is still in line
> with [2].
> 
> 
> [1] https://www.redhat.com/archives/libvir-list/2020-November/msg01409.html
> [2] https://gitlab.com/libvirt/libvirt/-/issues/7
> 
> 
> Daniel Henrique Barboza (9):
>    domain_conf.c: move boot related timeouts check to validate callback
>    domain_conf.c: move primary video check to validate callback
>    domain_conf.c: move QXL attributes check to
>      virDomainVideoDefValidate()
>    domain_conf.c: move vendor, product and tray checks to validate
>      callback
>    domain_conf.c: move smartcard address check to validate callback
>    domain_conf.c: move blkio path check to validate callback
>    domain_conf.c: move virDomainPCIControllerOpts checks to validate
>      callback
>    domain_conf.c: move pci-root/pcie-root address check to validateCB
>    domain_conf.c: move idmapEntry checks to validate callback
> 
>   src/conf/domain_conf.c                        | 353 +++++++++++-------
>   tests/qemuxml2argvdata/pci-root-address.err   |   2 +-
>   .../pseries-default-phb-numa-node.err         |   2 +-
>   .../video-multiple-primaries.err              |   1 +
>   .../video-multiple-primaries.xml              |  32 ++
>   tests/qemuxml2argvtest.c                      |  14 +-
>   6 files changed, 268 insertions(+), 136 deletions(-)
>   create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err
>   create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

But since we are splitting parsing and validation code can we use this 
chance and move validators out of domain_conf.c allowing it to be 
smaller in size?

Michal

Re: [PATCH v2 0/9] move checks from parse to validate callbacks

Posted by Daniel Henrique Barboza 3 years, 4 months ago


On 12/8/20 7:22 AM, Michal Privoznik wrote:
> On 12/7/20 2:54 PM, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This is the respin of [1] after the reviews from Michal.
>> Although we're pushing code to validate callbacks instead of
>> post parse functions, the idea and motivation is still in line
>> with [2].
>>
>>
>> [1] https://www.redhat.com/archives/libvir-list/2020-November/msg01409.html
>> [2] https://gitlab.com/libvirt/libvirt/-/issues/7
>>
>>
>> Daniel Henrique Barboza (9):
>>    domain_conf.c: move boot related timeouts check to validate callback
>>    domain_conf.c: move primary video check to validate callback
>>    domain_conf.c: move QXL attributes check to
>>      virDomainVideoDefValidate()
>>    domain_conf.c: move vendor, product and tray checks to validate
>>      callback
>>    domain_conf.c: move smartcard address check to validate callback
>>    domain_conf.c: move blkio path check to validate callback
>>    domain_conf.c: move virDomainPCIControllerOpts checks to validate
>>      callback
>>    domain_conf.c: move pci-root/pcie-root address check to validateCB
>>    domain_conf.c: move idmapEntry checks to validate callback
>>
>>   src/conf/domain_conf.c                        | 353 +++++++++++-------
>>   tests/qemuxml2argvdata/pci-root-address.err   |   2 +-
>>   .../pseries-default-phb-numa-node.err         |   2 +-
>>   .../video-multiple-primaries.err              |   1 +
>>   .../video-multiple-primaries.xml              |  32 ++
>>   tests/qemuxml2argvtest.c                      |  14 +-
>>   6 files changed, 268 insertions(+), 136 deletions(-)
>>   create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.err
>>   create mode 100644 tests/qemuxml2argvdata/video-multiple-primaries.xml
>>
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>


Thanks for the review!

> 
> But since we are splitting parsing and validation code can we use this chance and move validators out of domain_conf.c allowing it to be smaller in size?


Makes sense. I can create a 'domain_validate.c' and start moving stuff there,
like I did previously with QEMU driver validations and qemu_validate.c.
First order of business would be to move the validations I already played
with in this series to this new file.


What do you think?



DHB

> 
> Michal
>

Re: [PATCH v2 0/9] move checks from parse to validate callbacks

Posted by Michal Privoznik 3 years, 4 months ago

On 12/8/20 6:45 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 12/8/20 7:22 AM, Michal Privoznik wrote:

>>
>> But since we are splitting parsing and validation code can we use this 
>> chance and move validators out of domain_conf.c allowing it to be 
>> smaller in size?
> 
> 
> Makes sense. I can create a 'domain_validate.c' and start moving stuff 
> there,
> like I did previously with QEMU driver validations and qemu_validate.c.
> First order of business would be to move the validations I already played
> with in this series to this new file.
> 
> 
> What do you think?

Yes, qemu_validate.c example was what I had on mind. I don't have 
preference whether to move everything at once or by smaller pieces (how 
small?). While the latter might look like easier to review, not really 
since git diff has now --color-moved. That way a reviewer can verify 
that a commit is just moving code.

Michal

Re: [PATCH v2 0/9] move checks from parse to validate callbacks

Posted by Daniel Henrique Barboza 3 years, 4 months ago

On 12/8/20 4:58 PM, Michal Privoznik wrote:
> On 12/8/20 6:45 PM, Daniel Henrique Barboza wrote:
>>
>>
>> On 12/8/20 7:22 AM, Michal Privoznik wrote:
> 
>>>
>>> But since we are splitting parsing and validation code can we use this chance and move validators out of domain_conf.c allowing it to be smaller in size?
>>
>>
>> Makes sense. I can create a 'domain_validate.c' and start moving stuff there,
>> like I did previously with QEMU driver validations and qemu_validate.c.
>> First order of business would be to move the validations I already played
>> with in this series to this new file.
>>
>>
>> What do you think?
> 
> Yes, qemu_validate.c example was what I had on mind. I don't have preference whether to move everything at once or by smaller pieces (how small?). While the latter might look like easier to review, not really since git diff has now --color-moved. That way a reviewer can verify that a commit is just moving code.

In fact, I'm respining this series already creating domain_validate.c right
from the start. I find this way more sensible than adding code to domain_conf.c
just to move to a new file afterwards. I'll post a v3 shortly. I'll keep your
R-bs in the patches you already reviewed in v2 (the changes aren't worth
reviewing it again).

About how we're going to move the validations, I'll do what I did with
qemu_validate.c. For example, move all static functions that is called by
virDomainDefValidateInternal() in a single punch, then move
virDomainDefValidateInternal(). Same thing with other relevant validation
functions. This can be done in a follow-up series after this one.

DHB

> 
> Michal
>