5:44 p.m.
On 01/08/2016 06:35 AM, Peter Krempa wrote:
On Thu, Jan 07, 2016 at 22:50:06 -0500, Cole Robinson wrote:
> In order to make this work, we need to short circuit the normal
> virDomainDefPostParse ordering, and manually add stock devices
> ourselves, since we need them in the XML before assigning addresses.
>
> There's a bit of test suite churn due to extra XML output, and validation
> happening at different call sites, but it all looks correct to me.
>
> There's still quite a few manual callers of qemuDomainAssignAddresses
> that could be dropped too but it would need additional testing.
> ---
> src/qemu/qemu_domain.c | 10 +++++++
> src/qemu/qemu_driver.c | 9 ------
> .../qemuxml2argvdata/qemuxml2argv-pseries-disk.xml | 4 ++-
> tests/qemuxml2argvtest.c | 12 ++------
> .../qemuxml2xmlout-channel-virtio-auto.xml | 9 +++---
> .../qemuxml2xmlout-disk-scsi-vscsi.xml | 35 ++++++++++++++++++++++
> .../qemuxml2xmlout-panic-pseries.xml | 30 +++++++++++++++++++
> .../qemuxml2xmlout-pseries-panic-missing.xml | 4 +--
> .../qemuxml2xmlout-pseries-panic-no-address.xml | 4 +--
> tests/qemuxml2xmltest.c | 10 +++++--
> 10 files changed, 97 insertions(+), 30 deletions(-)
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-vscsi.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-panic-pseries.xml
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index a981310..e0520ab 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1249,6 +1249,16 @@ qemuDomainDefPostParse(virDomainDefPtr def,
> if (virSecurityManagerVerify(driver->securityManager, def) < 0)
> goto cleanup;
>
> + /* Device defaults are normally set after calling the driver specific
> + PostParse routine (this function), but we need them earlier. */
> + if (virDomainDefPostParseDevices(def, caps, driver->xmlopt) < 0)
> + goto cleanup;
NACK to this, this would create a possibly dangerous situation by
encouraging others to call the post parse callback handlers from
themeself.
What is dangerous about this? These operations are idempotent, and if they
ever failed to be idempotent, many things would break. Consider that
DefPostParseDevices is called every time a VM is redefined, or XML is read
from disk at daemon startup. Many xml2xml tests would likely fail as well.
Now is this change in good taste? debatable :)
If it's necessary to do two passes of post parse stuff, then
please add
a new callback pointer for it.
Let's say we change things to do:
virDomainDefPostParse() {
domainPostParseCallback1();
virDomainDefPostParseDevices();
virDomainDefPostParseInternal();
domainPostParseCallback2();
}
... how do we know the generic bits don't need to be called after Callback2?
We don't really. Especially because so much of the generic bits is just about
validation checking, Callback2 may have changed some bit that the generic code
needs to validate. So then maybe we do:
virDomainDefPostParse() {
domainPostParseCallback1();
virDomainDefPostParseDevices();
virDomainDefPostParseInternal();
domainPostParseCallback2();
virDomainDefPostParseDevices();
virDomainDefPostParseInternal();
}
But that's basically my proposed patch, except encoded generically rather than
to the only driver that currently needs.
Granted all that is hypothetical, but the point is that there isn't any clear
line that we can say 'this is for pass #1, this is for pass #2' except on a
case by case driver basis, which suggests to me it's a confusing thing to put
in generic code. If we accept the premise that the generic PostParse functions
must be idempotent, then it seems fine to me to handle any special ordering in
just one driver PostParse callback.
sidenote: IMO we _do_ need a separate PostParse entry point though, but
strictly to implement something like virDomainDefValidate() which always runs
at the end of PostParse. All it does is check for invalid config, and doesn't
change any settings. The goal would be to move as much validation as possible
out of the current PostParse and XML parsing bits into that function, so it
can be shared by impls that populate the DomainDef directly. Things seem like
they are already moving in that direction with much of the PostParse bits
already, but the validation vs. stateful bit isn't explicit. However that's a
much bigger project, and it's completely orthogonal to this patch since it
wouldn't solve my issue.
Thanks,
Cole