src/conf/numa_conf.c | 19 + src/conf/numa_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 575 +--------- src/qemu/qemu_command.h | 1 + src/qemu/qemu_domain.c | 983 ++++++++++++++++-- tests/qemuhotplugtest.c | 10 + tests/qemumemlocktest.c | 16 +- .../default-video-type-aarch64.xml | 2 +- .../default-video-type-ppc64.xml | 2 +- .../default-video-type-s390x.xml | 2 +- .../graphics-egl-headless.args | 2 +- .../graphics-spice-egl-headless.args | 2 +- .../graphics-vnc-egl-headless.args | 2 +- tests/qemuxml2argvtest.c | 80 +- ...ault-video-type-aarch64.aarch64-latest.xml | 4 +- .../default-video-type-ppc64.ppc64-latest.xml | 4 +- .../default-video-type-s390x.s390x-latest.xml | 4 +- tests/qemuxml2xmltest.c | 244 +++-- 19 files changed, 1246 insertions(+), 709 deletions(-)
(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.
-------
Cole also mentioned that the machine features validation was a good
place to start. I took it a step further and did it across the
board on all qemu_command.c.
This series didn't create any new validation, just moved them
to domain define time. Any other outcome is unintended.
Not all cases where covered in this work. For a first version
I decided to do only the most trivial cases. These are the
other cases I left out for another day:
- all verifications contained in functions that are also called
by qemu_hotplug.c. This means that the hotplug process will also
use the validation code, and we can't just move it to qemu_domain.c.
Besides, not all validation done in domain define time applies
to hotplug, so it's not simply a matter of calling the same function
from qemu_domain in qemu_hotplug. I have patches that moved the
verification of all virtio options to qemu_domain.c, while also
considering qemu_hotplug validation. In the end I decided to leave
it away from this work for now because (1) it took 8 patches just
for virtio case alone and (2) there are a lot of these cases in
qemu_command.c and it would be too much to do it all at in this
same series.
- 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.
- SHMem: part of SHMem validation is being used by hotplug code.
I could've moved the non-hotplug validation to define time,
instead I decided it's better to to leave it to do it all at
once in the future.
- DBus-VMState: validation of this fella must be done by first
querying if the hash vm->priv->dbusVMState has elements.
qemuDomainDefValidate does not have 'vm->priv' in it's API,
meaning that I would need to either change the API to include
it (which means changing domainValidateCallback), or do the
validation by another branch where I can get access to vm->priv.
- vmcoreinfo: there are no challenges in moving vmcoreinfo
validation to qemuDomainDefValidate. The problem were the
unit tests. Moving this validation to domain define time
breaks 925 tests on qemuxml2xmltest.c, including tests labelled
as 'minimal' which should pass under any circurstances, as
far as I understand. It is possible to just shoehorn
QEMU_CAPS_DEVICE_VMCOREINFO in the qemuCaps of all tests, but
this would tamper the idea of having to deal with NONE or
LATEST or a specific set of capabilities for certain tests.
Another case I would rather discuss separately.
[1] https://www.redhat.com/archives/libvir-list/2019-October/msg00568.html
Daniel Henrique Barboza (26):
qemu_command.c: move PSeries features validation to qemu_domain.c
qemu_command.c: move mem.nosharepages validation to qemu_domain.c
qemu_command.c: move validation of vmport to qemu_domain.c
qemu_command.c: move nvdimm validation to qemu_domain.c
qemu_command.c: move I/O APIC validation to qemu_domain.c
numa_conf: add virDomainNumaNodesDistancesAreBeingSet() helper
qemu_command.c move NUMA validation to qemu_domain.c
qemu_command.c: move NVRAM validation to qemu_domain.c
qemu_command: move qemuBuildSoundDevStr caps validation to qemu_domain
qemu_command.c: move sound codec validation to qemu_domain.c
qemu_command: move qemuBuildHubDevStr caps validation to qemu_domain
qemu_command: move qemuBuildChrChardevStr caps validation to
qemu_domain
qemu: move qemuBuildHostdevCommandLine caps validation to qemu_domain
qemu_command.c: move vmGenID validation to qemu_domain.c
qemu: move qemuBuildSgaCommandLine validation to qemu_domain.c
qemu: move virDomainClockDef validation to qemu_domain.c
qemu: move qemuBuildPMCommandLine validation to qemu_domain.c
qemu: move qemuBuildBootCommandLine validation to qemu_domain.c
qemu_command.c: move pcihole64 validation to qemu_domain.c
qemu: move qemuBuildGraphicsSDLCommandLine validation to qemu_domain.c
qemu: move qemuBuildGraphicsVNCCommandLine validation to qemu_domain.c
qemu: move qemuBuildGraphicsSPICECommandLine validation to
qemu_domain.c
qemu: move qemuBuildGraphicsEGLHeadlessCommandLine validation to
qemu_domain.c
qemu: move qemuBuildSmartcardCommandLine validation to qemu_domain.c
qemu: move qemuBuildConsoleCommandLine validation to qemu_domain.c
qemu: move qemuBuildTPMDevStr TPM validation to qemu_domain.c
src/conf/numa_conf.c | 19 +
src/conf/numa_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 575 +---------
src/qemu/qemu_command.h | 1 +
src/qemu/qemu_domain.c | 983 ++++++++++++++++--
tests/qemuhotplugtest.c | 10 +
tests/qemumemlocktest.c | 16 +-
.../default-video-type-aarch64.xml | 2 +-
.../default-video-type-ppc64.xml | 2 +-
.../default-video-type-s390x.xml | 2 +-
.../graphics-egl-headless.args | 2 +-
.../graphics-spice-egl-headless.args | 2 +-
.../graphics-vnc-egl-headless.args | 2 +-
tests/qemuxml2argvtest.c | 80 +-
...ault-video-type-aarch64.aarch64-latest.xml | 4 +-
.../default-video-type-ppc64.ppc64-latest.xml | 4 +-
.../default-video-type-s390x.s390x-latest.xml | 4 +-
tests/qemuxml2xmltest.c | 244 +++--
19 files changed, 1246 insertions(+), 709 deletions(-)
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
offlist. Wow this looks awesome, thanks for tackling it! I'll give the cover letter a response tomorrow when I've had time to digest it On 12/9/19 6:15 PM, 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. > ------- > > Cole also mentioned that the machine features validation was a good > place to start. I took it a step further and did it across the > board on all qemu_command.c. > > This series didn't create any new validation, just moved them > to domain define time. Any other outcome is unintended. > > > Not all cases where covered in this work. For a first version > I decided to do only the most trivial cases. These are the > other cases I left out for another day: > > - all verifications contained in functions that are also called > by qemu_hotplug.c. This means that the hotplug process will also > use the validation code, and we can't just move it to qemu_domain.c. > Besides, not all validation done in domain define time applies > to hotplug, so it's not simply a matter of calling the same function > from qemu_domain in qemu_hotplug. I have patches that moved the > verification of all virtio options to qemu_domain.c, while also > considering qemu_hotplug validation. In the end I decided to leave > it away from this work for now because (1) it took 8 patches just > for virtio case alone and (2) there are a lot of these cases in > qemu_command.c and it would be too much to do it all at in this > same series. > > - 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. > > - SHMem: part of SHMem validation is being used by hotplug code. > I could've moved the non-hotplug validation to define time, > instead I decided it's better to to leave it to do it all at > once in the future. > > - DBus-VMState: validation of this fella must be done by first > querying if the hash vm->priv->dbusVMState has elements. > qemuDomainDefValidate does not have 'vm->priv' in it's API, > meaning that I would need to either change the API to include > it (which means changing domainValidateCallback), or do the > validation by another branch where I can get access to vm->priv. > > - vmcoreinfo: there are no challenges in moving vmcoreinfo > validation to qemuDomainDefValidate. The problem were the > unit tests. Moving this validation to domain define time > breaks 925 tests on qemuxml2xmltest.c, including tests labelled > as 'minimal' which should pass under any circurstances, as > far as I understand. It is possible to just shoehorn > QEMU_CAPS_DEVICE_VMCOREINFO in the qemuCaps of all tests, but > this would tamper the idea of having to deal with NONE or > LATEST or a specific set of capabilities for certain tests. > Another case I would rather discuss separately. > > > [1] https://www.redhat.com/archives/libvir-list/2019-October/msg00568.html > > Daniel Henrique Barboza (26): > qemu_command.c: move PSeries features validation to qemu_domain.c > qemu_command.c: move mem.nosharepages validation to qemu_domain.c > qemu_command.c: move validation of vmport to qemu_domain.c > qemu_command.c: move nvdimm validation to qemu_domain.c > qemu_command.c: move I/O APIC validation to qemu_domain.c > numa_conf: add virDomainNumaNodesDistancesAreBeingSet() helper > qemu_command.c move NUMA validation to qemu_domain.c > qemu_command.c: move NVRAM validation to qemu_domain.c > qemu_command: move qemuBuildSoundDevStr caps validation to qemu_domain > qemu_command.c: move sound codec validation to qemu_domain.c > qemu_command: move qemuBuildHubDevStr caps validation to qemu_domain > qemu_command: move qemuBuildChrChardevStr caps validation to > qemu_domain > qemu: move qemuBuildHostdevCommandLine caps validation to qemu_domain > qemu_command.c: move vmGenID validation to qemu_domain.c > qemu: move qemuBuildSgaCommandLine validation to qemu_domain.c > qemu: move virDomainClockDef validation to qemu_domain.c > qemu: move qemuBuildPMCommandLine validation to qemu_domain.c > qemu: move qemuBuildBootCommandLine validation to qemu_domain.c > qemu_command.c: move pcihole64 validation to qemu_domain.c > qemu: move qemuBuildGraphicsSDLCommandLine validation to qemu_domain.c > qemu: move qemuBuildGraphicsVNCCommandLine validation to qemu_domain.c > qemu: move qemuBuildGraphicsSPICECommandLine validation to > qemu_domain.c > qemu: move qemuBuildGraphicsEGLHeadlessCommandLine validation to > qemu_domain.c > qemu: move qemuBuildSmartcardCommandLine validation to qemu_domain.c > qemu: move qemuBuildConsoleCommandLine validation to qemu_domain.c > qemu: move qemuBuildTPMDevStr TPM validation to qemu_domain.c > > src/conf/numa_conf.c | 19 + > src/conf/numa_conf.h | 2 + > src/libvirt_private.syms | 1 + > src/qemu/qemu_command.c | 575 +--------- > src/qemu/qemu_command.h | 1 + > src/qemu/qemu_domain.c | 983 ++++++++++++++++-- > tests/qemuhotplugtest.c | 10 + > tests/qemumemlocktest.c | 16 +- > .../default-video-type-aarch64.xml | 2 +- > .../default-video-type-ppc64.xml | 2 +- > .../default-video-type-s390x.xml | 2 +- > .../graphics-egl-headless.args | 2 +- > .../graphics-spice-egl-headless.args | 2 +- > .../graphics-vnc-egl-headless.args | 2 +- > tests/qemuxml2argvtest.c | 80 +- > ...ault-video-type-aarch64.aarch64-latest.xml | 4 +- > .../default-video-type-ppc64.ppc64-latest.xml | 4 +- > .../default-video-type-s390x.s390x-latest.xml | 4 +- > tests/qemuxml2xmltest.c | 244 +++-- > 19 files changed, 1246 insertions(+), 709 deletions(-) > - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/9/19 6:15 PM, 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. > ------- > > Cole also mentioned that the machine features validation was a good > place to start. I took it a step further and did it across the > board on all qemu_command.c. > > This series didn't create any new validation, just moved them > to domain define time. Any other outcome is unintended. > > > Not all cases where covered in this work. For a first version > I decided to do only the most trivial cases. These are the > other cases I left out for another day: > > - all verifications contained in functions that are also called > by qemu_hotplug.c. This means that the hotplug process will also > use the validation code, and we can't just move it to qemu_domain.c. > Besides, not all validation done in domain define time applies > to hotplug, so it's not simply a matter of calling the same function > from qemu_domain in qemu_hotplug. I have patches that moved the > verification of all virtio options to qemu_domain.c, while also > considering qemu_hotplug validation. In the end I decided to leave > it away from this work for now because (1) it took 8 patches just > for virtio case alone and (2) there are a lot of these cases in > qemu_command.c and it would be too much to do it all at in this > same series. > > - 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. > > - SHMem: part of SHMem validation is being used by hotplug code. > I could've moved the non-hotplug validation to define time, > instead I decided it's better to to leave it to do it all at > once in the future. > > - DBus-VMState: validation of this fella must be done by first > querying if the hash vm->priv->dbusVMState has elements. > qemuDomainDefValidate does not have 'vm->priv' in it's API, > meaning that I would need to either change the API to include > it (which means changing domainValidateCallback), or do the > validation by another branch where I can get access to vm->priv. > > - vmcoreinfo: there are no challenges in moving vmcoreinfo > validation to qemuDomainDefValidate. The problem were the > unit tests. Moving this validation to domain define time > breaks 925 tests on qemuxml2xmltest.c, including tests labelled > as 'minimal' which should pass under any circurstances, as > far as I understand. It is possible to just shoehorn > QEMU_CAPS_DEVICE_VMCOREINFO in the qemuCaps of all tests, but > this would tamper the idea of having to deal with NONE or > LATEST or a specific set of capabilities for certain tests. > Another case I would rather discuss separately. > > > [1] https://www.redhat.com/archives/libvir-list/2019-October/msg00568.html > > Daniel Henrique Barboza (26): > qemu_command.c: move PSeries features validation to qemu_domain.c > qemu_command.c: move mem.nosharepages validation to qemu_domain.c > qemu_command.c: move validation of vmport to qemu_domain.c > qemu_command.c: move nvdimm validation to qemu_domain.c > qemu_command.c: move I/O APIC validation to qemu_domain.c > numa_conf: add virDomainNumaNodesDistancesAreBeingSet() helper > qemu_command.c move NUMA validation to qemu_domain.c > qemu_command.c: move NVRAM validation to qemu_domain.c > qemu_command: move qemuBuildSoundDevStr caps validation to qemu_domain > qemu_command.c: move sound codec validation to qemu_domain.c > qemu_command: move qemuBuildHubDevStr caps validation to qemu_domain > qemu_command: move qemuBuildChrChardevStr caps validation to > qemu_domain > qemu: move qemuBuildHostdevCommandLine caps validation to qemu_domain > qemu_command.c: move vmGenID validation to qemu_domain.c > qemu: move qemuBuildSgaCommandLine validation to qemu_domain.c > qemu: move virDomainClockDef validation to qemu_domain.c > qemu: move qemuBuildPMCommandLine validation to qemu_domain.c > qemu: move qemuBuildBootCommandLine validation to qemu_domain.c > qemu_command.c: move pcihole64 validation to qemu_domain.c > qemu: move qemuBuildGraphicsSDLCommandLine validation to qemu_domain.c > qemu: move qemuBuildGraphicsVNCCommandLine validation to qemu_domain.c > qemu: move qemuBuildGraphicsSPICECommandLine validation to > qemu_domain.c > qemu: move qemuBuildGraphicsEGLHeadlessCommandLine validation to > qemu_domain.c > qemu: move qemuBuildSmartcardCommandLine validation to qemu_domain.c > qemu: move qemuBuildConsoleCommandLine validation to qemu_domain.c > qemu: move qemuBuildTPMDevStr TPM validation to qemu_domain.c > Reviewed-by: Cole Robinson <crobinso@redhat.com> I standardized the subject prefixes to 'qemu: command:' and pushed them all, except 4, 23, and 24, the ones I commented on Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/9/19 6:15 PM, 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.
> -------
>
> Cole also mentioned that the machine features validation was a good
> place to start. I took it a step further and did it across the
> board on all qemu_command.c.
>
> This series didn't create any new validation, just moved them
> to domain define time. Any other outcome is unintended.
>
>
> Not all cases where covered in this work. For a first version
> I decided to do only the most trivial cases. These are the
> other cases I left out for another day:
>
Yes I think that's the right approach, handle the easy bits first and
deal the complicated bits in their own series. Some cases will likely
cause a lot of test suite churn, some cases may not even be worth it, we
will see!
> - all verifications contained in functions that are also called
> by qemu_hotplug.c. This means that the hotplug process will also
> use the validation code, and we can't just move it to qemu_domain.c.
> Besides, not all validation done in domain define time applies
> to hotplug, so it's not simply a matter of calling the same function
> from qemu_domain in qemu_hotplug. I have patches that moved the
> verification of all virtio options to qemu_domain.c, while also
> considering qemu_hotplug validation. In the end I decided to leave
> it away from this work for now because (1) it took 8 patches just
> for virtio case alone and (2) there are a lot of these cases in
> qemu_command.c and it would be too much to do it all at in this
> same series.
>
The hotplug code should already be calling into Validate code. When a
device is hotplugged via qemu_driver, we get:
qemu_driver.c
-> qemuDomainAttachDeviceFlags
-> qemuDomainAttachDeviceLiveAndConfig
-> virDomainDeviceDefParse
-> virDomainDeviceDefValidate
-> deviceValidateCallback
-> qemuDomainDeviceDefValidate
So if device validation is moved into the correct
qemuDomainDeviceDefValidateXXX function it will get called in the
hotplug path so they won't be lost.
> - 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.
>
Hmm I glanced at the qemuBuildCpuModelArgStr checks but it doesn't
strike me why those are an issue. Validate should always be triggered
after PostParse, so the ordering of those two shouldn't impact things
here. But I didn't attempt the change
> - SHMem: part of SHMem validation is being used by hotplug code.
> I could've moved the non-hotplug validation to define time,
> instead I decided it's better to to leave it to do it all at
> once in the future.
>
shmem is just another device, so it should be fine as long as its final
location is triggered by qemuDomainDeviceDefValidate
> - DBus-VMState: validation of this fella must be done by first
> querying if the hash vm->priv->dbusVMState has elements.
> qemuDomainDefValidate does not have 'vm->priv' in it's API,
> meaning that I would need to either change the API to include
> it (which means changing domainValidateCallback), or do the
> validation by another branch where I can get access to vm->priv.
>
Yeah this one is a little convoluted. The code is using the vm->priv
check to determine 'the user requested external slirp', which is really
a factor of interface type='user' + a qemu.conf setting, which we _can_
check at validate time against qemuCaps. But it's minor so I suggest
just ignoring it
> - vmcoreinfo: there are no challenges in moving vmcoreinfo
> validation to qemuDomainDefValidate. The problem were the
> unit tests. Moving this validation to domain define time
> breaks 925 tests on qemuxml2xmltest.c, including tests labelled
> as 'minimal' which should pass under any circurstances, as
> far as I understand. It is possible to just shoehorn
> QEMU_CAPS_DEVICE_VMCOREINFO in the qemuCaps of all tests, but
> this would tamper the idea of having to deal with NONE or
> LATEST or a specific set of capabilities for certain tests.
> Another case I would rather discuss separately.
>
I suspect a bug on your side. qemu_command.c has:
static int
qemuBuildVMCoreInfoCommandLine(virCommandPtr cmd,
const virDomainDef *def,
virQEMUCapsPtr qemuCaps)
{
virTristateSwitch vmci =
def->features[VIR_DOMAIN_FEATURE_VMCOREINFO];
if (vmci != VIR_TRISTATE_SWITCH_ON)
return 0;
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMCOREINFO)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("vmcoreinfo is not available "
"with this QEMU binary"));
return -1;
}
virCommandAddArgList(cmd, "-device", "vmcoreinfo", NULL);
return 0;
}
If you just copy the middle section, you'll get tons of failures. But
what you want to add is:
if (def->features[VIR_DOMAIN_FEATURE_VMCOREINFO] ==
VIR_TRISTATE_SWITCH_ON &&
!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMCOREINFO))
<error>
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/10/19 6:41 PM, Cole Robinson wrote: > On 12/9/19 6:15 PM, Daniel Henrique Barboza wrote: >> (series based on master commit 97cafa610ecf5) >> [...] > > The hotplug code should already be calling into Validate code. When a > device is hotplugged via qemu_driver, we get: > > qemu_driver.c > -> qemuDomainAttachDeviceFlags > -> qemuDomainAttachDeviceLiveAndConfig > -> virDomainDeviceDefParse > -> virDomainDeviceDefValidate > -> deviceValidateCallback > -> qemuDomainDeviceDefValidate > > So if device validation is moved into the correct > qemuDomainDeviceDefValidateXXX function it will get called in the > hotplug path so they won't be lost. Good to know. I assumed that the hotplug path was separated. Perhaps this info can be put somewhere in the docs folder as reference. This appears to be the kind of call hierarchy that doesn't change that often, so it wouldn't be a burden to keep the doc updated. > >> - 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. >> > > Hmm I glanced at the qemuBuildCpuModelArgStr checks but it doesn't > strike me why those are an issue. Validate should always be triggered > after PostParse, so the ordering of those two shouldn't impact things > here. But I didn't attempt the change > Unfortunately I've erased the error I was seeing in this case. I recall something to do with VIR_CPU_MODE_HOST_MODEL being set in qemuDomainDefSetDefaultCPU(), but TBH it's easier to just move the code again and see what happens. >> > > I suspect a bug on your side. qemu_command.c has: I'll attempt that in the next iterations of this work. Thanks, Daniel -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi, I failed to make a reservation about what I've done in patch in the cover letter. In the commit msg I mentioned about the XML files considering SPICE support as default to ppc64, aarch64 and s390 archs and fixing all the xmls. It is worth disclaiming that what I can assert to be true is the lack of SPICE support for ppc64. I can't put my money on the lack of SPICE for the other 2 archs - what I did in the patch was to assume that the emulator capabilities for aarch64 and s390, that didn't report SPICE support, was accurate. I think it's an educated guess, but a guess nevertheless. Thanks, DHB On 12/9/19 8:15 PM, 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. > ------- > > Cole also mentioned that the machine features validation was a good > place to start. I took it a step further and did it across the > board on all qemu_command.c. > > This series didn't create any new validation, just moved them > to domain define time. Any other outcome is unintended. > > > Not all cases where covered in this work. For a first version > I decided to do only the most trivial cases. These are the > other cases I left out for another day: > > - all verifications contained in functions that are also called > by qemu_hotplug.c. This means that the hotplug process will also > use the validation code, and we can't just move it to qemu_domain.c. > Besides, not all validation done in domain define time applies > to hotplug, so it's not simply a matter of calling the same function > from qemu_domain in qemu_hotplug. I have patches that moved the > verification of all virtio options to qemu_domain.c, while also > considering qemu_hotplug validation. In the end I decided to leave > it away from this work for now because (1) it took 8 patches just > for virtio case alone and (2) there are a lot of these cases in > qemu_command.c and it would be too much to do it all at in this > same series. > > - 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. > > - SHMem: part of SHMem validation is being used by hotplug code. > I could've moved the non-hotplug validation to define time, > instead I decided it's better to to leave it to do it all at > once in the future. > > - DBus-VMState: validation of this fella must be done by first > querying if the hash vm->priv->dbusVMState has elements. > qemuDomainDefValidate does not have 'vm->priv' in it's API, > meaning that I would need to either change the API to include > it (which means changing domainValidateCallback), or do the > validation by another branch where I can get access to vm->priv. > > - vmcoreinfo: there are no challenges in moving vmcoreinfo > validation to qemuDomainDefValidate. The problem were the > unit tests. Moving this validation to domain define time > breaks 925 tests on qemuxml2xmltest.c, including tests labelled > as 'minimal' which should pass under any circurstances, as > far as I understand. It is possible to just shoehorn > QEMU_CAPS_DEVICE_VMCOREINFO in the qemuCaps of all tests, but > this would tamper the idea of having to deal with NONE or > LATEST or a specific set of capabilities for certain tests. > Another case I would rather discuss separately. > > > [1] https://www.redhat.com/archives/libvir-list/2019-October/msg00568.html > > Daniel Henrique Barboza (26): > qemu_command.c: move PSeries features validation to qemu_domain.c > qemu_command.c: move mem.nosharepages validation to qemu_domain.c > qemu_command.c: move validation of vmport to qemu_domain.c > qemu_command.c: move nvdimm validation to qemu_domain.c > qemu_command.c: move I/O APIC validation to qemu_domain.c > numa_conf: add virDomainNumaNodesDistancesAreBeingSet() helper > qemu_command.c move NUMA validation to qemu_domain.c > qemu_command.c: move NVRAM validation to qemu_domain.c > qemu_command: move qemuBuildSoundDevStr caps validation to qemu_domain > qemu_command.c: move sound codec validation to qemu_domain.c > qemu_command: move qemuBuildHubDevStr caps validation to qemu_domain > qemu_command: move qemuBuildChrChardevStr caps validation to > qemu_domain > qemu: move qemuBuildHostdevCommandLine caps validation to qemu_domain > qemu_command.c: move vmGenID validation to qemu_domain.c > qemu: move qemuBuildSgaCommandLine validation to qemu_domain.c > qemu: move virDomainClockDef validation to qemu_domain.c > qemu: move qemuBuildPMCommandLine validation to qemu_domain.c > qemu: move qemuBuildBootCommandLine validation to qemu_domain.c > qemu_command.c: move pcihole64 validation to qemu_domain.c > qemu: move qemuBuildGraphicsSDLCommandLine validation to qemu_domain.c > qemu: move qemuBuildGraphicsVNCCommandLine validation to qemu_domain.c > qemu: move qemuBuildGraphicsSPICECommandLine validation to > qemu_domain.c > qemu: move qemuBuildGraphicsEGLHeadlessCommandLine validation to > qemu_domain.c > qemu: move qemuBuildSmartcardCommandLine validation to qemu_domain.c > qemu: move qemuBuildConsoleCommandLine validation to qemu_domain.c > qemu: move qemuBuildTPMDevStr TPM validation to qemu_domain.c > > src/conf/numa_conf.c | 19 + > src/conf/numa_conf.h | 2 + > src/libvirt_private.syms | 1 + > src/qemu/qemu_command.c | 575 +--------- > src/qemu/qemu_command.h | 1 + > src/qemu/qemu_domain.c | 983 ++++++++++++++++-- > tests/qemuhotplugtest.c | 10 + > tests/qemumemlocktest.c | 16 +- > .../default-video-type-aarch64.xml | 2 +- > .../default-video-type-ppc64.xml | 2 +- > .../default-video-type-s390x.xml | 2 +- > .../graphics-egl-headless.args | 2 +- > .../graphics-spice-egl-headless.args | 2 +- > .../graphics-vnc-egl-headless.args | 2 +- > tests/qemuxml2argvtest.c | 80 +- > ...ault-video-type-aarch64.aarch64-latest.xml | 4 +- > .../default-video-type-ppc64.ppc64-latest.xml | 4 +- > .../default-video-type-s390x.s390x-latest.xml | 4 +- > tests/qemuxml2xmltest.c | 244 +++-- > 19 files changed, 1246 insertions(+), 709 deletions(-) > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 07:22:37AM -0300, Daniel Henrique Barboza wrote: > Hi, > > I failed to make a reservation about what I've done in patch in the > cover letter. In the commit msg I mentioned about the XML files > considering SPICE support as default to ppc64, aarch64 and s390 archs and > fixing all the xmls. > > It is worth disclaiming that what I can assert to be true is the lack of > SPICE support for ppc64. I can't put my money on the lack of SPICE for > the other 2 archs - what I did in the patch was to assume that the emulator > capabilities for aarch64 and s390, that didn't report SPICE support, was > accurate. I think it's an educated guess, but a guess nevertheless. SPICE is present on arch64. 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
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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 09:58:38 +0000, Daniel Berrange wrote: > 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. Validation is not done on the persistent or status XML files exactly for this reason. It's re-done when starting the VM so this should not be an issue. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/10/19 5:56 AM, Peter Krempa wrote: > On Tue, Dec 10, 2019 at 09:58:38 +0000, Daniel Berrange wrote: >> 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. > > Validation is not done on the persistent or status XML files exactly for > this reason. > > It's re-done when starting the VM so this should not be an issue. > Yup, that's what the VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE flag is all about. It's used to skip validation at daemon startup, and various cases like DomainDefCopy Conceptually this moves closer to IMO the ideal model: 1) Serialize XML into C struct 2) Validate contents against hypervisor capabilities 3) Serialize C struct into command line string Which has benefits: simplifies code in #1 and #3 which is the interesting stuff, easier to test full code coverage of #1 and #3, gives us the option to move validation into domaincapabilities code so we can report to the user whether a feature will be rejected or not. Also I suspect if we moved to using virtblocks for cli stuff, untangling validation from the cli builder would be a step in that direction - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Dec 09, 2019 at 20:15:05 -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]: Nice work. Doing this was long overdue. My only suggestion is that after this we should move all validation into a separate file. qemu_domain was a code dumping place for a long time and since we now have a lot of common code moving it out would be benficial for cleaning up an making it more obvious. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 12/10/19 5:00 AM, Peter Krempa wrote: > On Mon, Dec 09, 2019 at 20:15:05 -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]: > > Nice work. Doing this was long overdue. My only suggestion is that after > this we should move all validation into a separate file. qemu_domain was > a code dumping place for a long time and since we now have a lot of > common code moving it out would be benficial for cleaning up an making > it more obvious. Got it. We can create this new file and move the validations from qemu_domain.c to the new file, before resuming moving more code out of qemu_command.c. That way we avoid moving the code twice. Thanks, DHB -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Dec 10, 2019 at 19:45:01 -0300, Daniel Henrique Barboza wrote: > > > On 12/10/19 5:00 AM, Peter Krempa wrote: > > On Mon, Dec 09, 2019 at 20:15:05 -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]: > > > > Nice work. Doing this was long overdue. My only suggestion is that after > > this we should move all validation into a separate file. qemu_domain was > > a code dumping place for a long time and since we now have a lot of > > common code moving it out would be benficial for cleaning up an making > > it more obvious. > > > Got it. We can create this new file and move the validations from qemu_domain.c > to the new file, before resuming moving more code out of qemu_command.c. That way > we avoid moving the code twice. Since you've got patches already it's okay to do it in two steps. It will be much appreciated if you move it to a new file, but it's not required for this series. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.