Re: [libvirt] [PATCH v5 14/16] qemu: Complete PCI command checks to controller def validate
On 01/28/2018 03:48 AM, Michal Privoznik wrote:
On 01/06/2018 12:47 AM, John Ferlan wrote:
> Move the qemuCaps checks over to qemuDomainControllerDefValidatePCI.
>
> This requires two test updates in order to set the correct capability
> bit for an xml2xml test as well as setting up the similar capability
> for the pseries memlocktest.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 70 +------------------------------------------
> src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++--
> tests/qemumemlocktest.c | 14 +++++++++
> tests/qemuxml2xmltest.c | 5 +++-
> 4 files changed, 97 insertions(+), 72 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 0dbc73399..7a138f921 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2726,23 +2726,11 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
>
> switch ((virDomainControllerModelPCI) def->model) {
> case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
> - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("the pci-bridge controller "
> - "is not supported in this QEMU
binary"));
> - goto error;
> - }
> virBufferAsprintf(&buf, "%s,chassis_nr=%d,id=%s",
> modelName, pciopts->chassisNr,
> def->info.alias);
> break;
> case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
> - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("the pxb controller "
> - "is not supported in this QEMU
binary"));
> - goto error;
> - }
> virBufferAsprintf(&buf, "%s,bus_nr=%d,id=%s",
> modelName, pciopts->busNr,
> def->info.alias);
I'm worried that we cannot do this. As I say in one of of comments to
previous patches - TOCTOU problem. What if somebody downgrades qemu
between define & start times? In this light, I no longer think we can do
03/16, can we?
Michal
I assume you mean 2 & 3 then? Still TOCTOU would be true for any
Validate vs. Run checks.
Long ago, there was some review or comment made where there was a desire
to move many of the domain validation checks from command line building
into a separate phase so that the only thing that fails during command
line building would be something that really caused the command line
build to fail, not some configuration error.
Doing that was tricky due to the domains disappearing problem. IIRC
that's essentially why the Validation code was created.
As for someone downgrading - well they're either going to fail on their
next edit of their guest or they're going to fail when they try to run,
right? It won't be a libvirt failure, it'll be a qemu failure. Still, if
the run fails after a qemu downgrade because someone didn't validate
that the guest that they changed to take advantage of some feature in
the qemu that they now have downgraded from, then who's "problem" is
that? <tongue-in-cheek> Ooohh - excellent qemu now supports X, let's
upgrade and use X. So update qemu and alter the guest to use X. After
using X it's determined, damn, I don't like X, so I'm going to downgrade
to ensure X isn't used. Now if they don't update their guest to remove X
as well, then the next start will fail in qemu because X doesn't exist.
Of course they'll blame and curse libvirt, but maybe, just maybe,
they'll recall they forgot to remove X from the guest. Who's fault is
that? </tongue-in-cheek>
Look if the desire is to just keep all the controller validation checks
at run time, then fine - I can drop this series. That's fine.
Personally, it felt better doing things during validate.
Tks -
John