Re: [libvirt] [PATCH v1 00/26] move qemucaps validations from qemu_command to qemu_domain
On Mon, Dec 09, 2019 at 08:15:05PM -0300, Daniel Henrique Barboza wrote:
(series based on master commit 97cafa610ecf5)
This work was proposed by Cole in [1]. This is Cole's reasoning for
it, copy/pasted from [1]:
-------
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.
-------
I've not looked at the patches, but surely moving this validate from
start time, to be define time errors is going to cause functional
regressions in our ABI behaviour.
My libvirtd daemons installs have many XML files defined which will
fail validation at various points in time, depending on what QEMU
builds I happen to have deployed. I only need them to pass the
validation when actually starting the VM.
I understand the motivation of making the CLI code cleaner, but
there is a flip side in that we're now separating logic that is
conceptually quite closely related. The CLI code makes various
assumptions and currently those assumptions are validated at
the same place. If we move that validation elsewhere, we increase
the liklihood that we're going to have bugs where the validation
code is not matching the assumptions the CLI code is making.
- moving CPU Model validation is trickier than the rest because
there are code in DefPostParse() that makes CPU Model changes that
are then validated in qemu_command.c. Moving the validation to define
time doesn't cut in this case - the validation is considering
PostParse changes in the CPU Model and some of the will fail if
done by qemuDomainDefValidate time. I didn't think too much about
how to handle this case, but given that the approach would be
different from the other cases handled here, I left it out too.
IMHO this should only ever be validated at start time. The host CPU
features seen at define time should not be assumed to be the same
as those that will be present at start time.
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 :|