[PATCH v1 00/24] move more validations to qemu_validate.c

Daniel Henrique Barboza posted 24 patches 1 week ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20201014204307.631746-1-danielhb413@gmail.com
src/qemu/qemu_command.c                       | 379 +--------
src/qemu/qemu_command.h                       |   4 +-
src/qemu/qemu_hotplug.c                       |   4 +-
src/qemu/qemu_validate.c                      | 740 ++++++++++++++----
tests/qemuhotplugtest.c                       |   1 +
.../disk-sata-incompatible-address.err        |   2 +-
.../net-virtio-rxqueuesize-invalid-size.err   |   2 +-
.../pseries-panic-address.err                 |   2 +-
tests/qemuxml2argvtest.c                      |  28 +-
tests/qemuxml2xmltest.c                       |  62 +-
10 files changed, 706 insertions(+), 518 deletions(-)

[PATCH v1 00/24] move more validations to qemu_validate.c

Posted by Daniel Henrique Barboza 1 week ago
Hi,

This is another step following up the work done in [1].

Recapping, the idea is to move any validation that can
be done in parse/define time to qemu_validate.c instead
of let it sit inside a command line build function in
qemu_command.c. I'll again copy/paste the reasoning Cole
Robinson gave for this work back then (see [2] for more
info):

-------
The benefits of moving to validate time is that XML is rejected by
'virsh define' rather than at 'virsh start' time. It also makes it easier
to follow the cli building code, and makes it easier to verify qemu_command.c
test suite code coverage for the important stuff like covering every CLI
option. It's also a good intermediate step for sharing validation with
domain capabilities building, like is done presently for rng models.
-------

Not all validations were moved with these series, but I covered most
of them.


One thing worth noticing is the existence of 'post start' validations,
most of them related to PCI addressing, and other notable cases such as
NUMA nodes and CPU models, that can't be moved to parse time. The reason
is that the QEMU driver will change them during start time.
I contemplated move them to the existing qemuBuildCommandLineValidate(),
but that would remove them from the hotplug path that uses the
qemuBuild*DevStr() functions. I don't have an easy answer on how to handle
these 'post start' validations ATM, so for now they remain in
qemu_command.c.


[1] https://www.redhat.com/archives/libvir-list/2019-December/msg00570.html
[2] https://www.redhat.com/archives/libvir-list/2019-October/msg00568.html

 src/qemu/qemu_command.c                       | 379 +--------
 src/qemu/qemu_command.h                       |   4 +-
 src/qemu/qemu_hotplug.c                       |   4 +-
 src/qemu/qemu_validate.c                      | 740 ++++++++++++++----
 tests/qemuhotplugtest.c                       |   1 +
 .../disk-sata-incompatible-address.err        |   2 +-
 .../net-virtio-rxqueuesize-invalid-size.err   |   2 +-
 .../pseries-panic-address.err                 |   2 +-
 tests/qemuxml2argvtest.c                      |  28 +-
 tests/qemuxml2xmltest.c                       |  62 +-
 10 files changed, 706 insertions(+), 518 deletions(-)

-- 
2.26.2

Re: [PATCH v1 00/24] move more validations to qemu_validate.c

Posted by Michal Privoznik 1 week ago
On 10/14/20 10:42 PM, Daniel Henrique Barboza wrote:
> Hi,
> 
> This is another step following up the work done in [1].
> 
> Recapping, the idea is to move any validation that can
> be done in parse/define time to qemu_validate.c instead
> of let it sit inside a command line build function in
> qemu_command.c. I'll again copy/paste the reasoning Cole
> Robinson gave for this work back then (see [2] for more
> info):
> 
> -------
> The benefits of moving to validate time is that XML is rejected by
> 'virsh define' rather than at 'virsh start' time. It also makes it easier
> to follow the cli building code, and makes it easier to verify qemu_command.c
> test suite code coverage for the important stuff like covering every CLI
> option. It's also a good intermediate step for sharing validation with
> domain capabilities building, like is done presently for rng models.
> -------
> 
> Not all validations were moved with these series, but I covered most
> of them.
> 
> 
> One thing worth noticing is the existence of 'post start' validations,
> most of them related to PCI addressing, and other notable cases such as
> NUMA nodes and CPU models, that can't be moved to parse time. The reason
> is that the QEMU driver will change them during start time.
> I contemplated move them to the existing qemuBuildCommandLineValidate(),
> but that would remove them from the hotplug path that uses the
> qemuBuild*DevStr() functions. I don't have an easy answer on how to handle
> these 'post start' validations ATM, so for now they remain in
> qemu_command.c.
> 
> 
> [1] https://www.redhat.com/archives/libvir-list/2019-December/msg00570.html
> [2] https://www.redhat.com/archives/libvir-list/2019-October/msg00568.html
> 
>   src/qemu/qemu_command.c                       | 379 +--------
>   src/qemu/qemu_command.h                       |   4 +-
>   src/qemu/qemu_hotplug.c                       |   4 +-
>   src/qemu/qemu_validate.c                      | 740 ++++++++++++++----
>   tests/qemuhotplugtest.c                       |   1 +
>   .../disk-sata-incompatible-address.err        |   2 +-
>   .../net-virtio-rxqueuesize-invalid-size.err   |   2 +-
>   .../pseries-panic-address.err                 |   2 +-
>   tests/qemuxml2argvtest.c                      |  28 +-
>   tests/qemuxml2xmltest.c                       |  62 +-
>   10 files changed, 706 insertions(+), 518 deletions(-)
> 

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

Michal

Re: [PATCH v1 00/24] move more validations to qemu_validate.c

Posted by Daniel Henrique Barboza 1 week ago

On 10/15/20 8:46 AM, Michal Privoznik wrote:
> On 10/14/20 10:42 PM, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This is another step following up the work done in [1].
>>
>> Recapping, the idea is to move any validation that can
>> be done in parse/define time to qemu_validate.c instead
>> of let it sit inside a command line build function in
>> qemu_command.c. I'll again copy/paste the reasoning Cole
>> Robinson gave for this work back then (see [2] for more
>> info):
>>
>> -------
>> The benefits of moving to validate time is that XML is rejected by
>> 'virsh define' rather than at 'virsh start' time. It also makes it easier
>> to follow the cli building code, and makes it easier to verify qemu_command.c
>> test suite code coverage for the important stuff like covering every CLI
>> option. It's also a good intermediate step for sharing validation with
>> domain capabilities building, like is done presently for rng models.
>> -------
>>
>> Not all validations were moved with these series, but I covered most
>> of them.
>>
>>
>> One thing worth noticing is the existence of 'post start' validations,
>> most of them related to PCI addressing, and other notable cases such as
>> NUMA nodes and CPU models, that can't be moved to parse time. The reason
>> is that the QEMU driver will change them during start time.
>> I contemplated move them to the existing qemuBuildCommandLineValidate(),
>> but that would remove them from the hotplug path that uses the
>> qemuBuild*DevStr() functions. I don't have an easy answer on how to handle
>> these 'post start' validations ATM, so for now they remain in
>> qemu_command.c.
>>
>>
>> [1] https://www.redhat.com/archives/libvir-list/2019-December/msg00570.html
>> [2] https://www.redhat.com/archives/libvir-list/2019-October/msg00568.html
>>
>>   src/qemu/qemu_command.c                       | 379 +--------
>>   src/qemu/qemu_command.h                       |   4 +-
>>   src/qemu/qemu_hotplug.c                       |   4 +-
>>   src/qemu/qemu_validate.c                      | 740 ++++++++++++++----
>>   tests/qemuhotplugtest.c                       |   1 +
>>   .../disk-sata-incompatible-address.err        |   2 +-
>>   .../net-virtio-rxqueuesize-invalid-size.err   |   2 +-
>>   .../pseries-panic-address.err                 |   2 +-
>>   tests/qemuxml2argvtest.c                      |  28 +-
>>   tests/qemuxml2xmltest.c                       |  62 +-
>>   10 files changed, 706 insertions(+), 518 deletions(-)
>>
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>


Thanks for the review! Applied all your suggestions in patches 03, 05, 08 and
15 and pushed.


DHB

> 
> Michal
>