Re: [libvirt] [PATCH v4 12/14] qemu_hotplug: Hotplug of reservations
On 04/14/2018 05:14 PM, John Ferlan wrote:
On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> When attaching a disk that requires pr-manager we might need to
> plug the pr-manager object too.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_hotplug.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
My opinion - combine w/ previous patch.
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 468153c79c..43bb910ed6 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -391,6 +391,29 @@ qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm,
> }
>
>
> +static int
> +qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm,
> + qemuDomainDiskPRDPtr prd,
> + virJSONValuePtr *propsret)
> +{
> + size_t i;
> +
> + *propsret = NULL;
> +
> + for (i = 0; i < vm->def->ndisks; i++) {
> + const virDomainDiskDef *domainDisk = vm->def->disks[i];
> +
> + if (virStoragePRDefIsManaged(domainDisk->src->pr)) {
> + /* qemu-pr-helper should be already started because
> + * another disk in domain requires it. */
> + return 0;
> + }
> + }
I don't understand the need for the above hunk - that was done in
patch11 wasn't it? This is why I like things combined - going back and
forth and changing logic as patches are developed means something may be
missed.
Okay the comment might be misleading. It should be s/started/added/.
Anwyay, when adding a disk that has PR configured we need to:
1) start pr-helper process (if we are the ones managing it),
2) construct JSON for pr-manager object,
3) add pr-mananager object on the monitor,
4) finally, add disk on the monitor.
And we need to do it in this order, otherwise qemu might try to connect
to not yet running process. Now, obviously, since there can be only one
managed pr-helper process per domain, there can be only one pr-manager
object. Therefore, when somebody is trying to hotplug a disk with PR
enabled & managed, but there already is a disk like that: step 1) turns
into no-op (see previous patch), and we also need steps 2) and 3) to
turn into no-op because the object is already in qemu. Trying to add it
again would result in error.
> +
> + return qemuBuildPRManagerInfoProps(prd, propsret);
> +}
> +
> +
> /**
> * qemuDomainAttachDiskGeneric:
> *
> @@ -411,13 +434,16 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
> bool driveAdded = false;
> bool secobjAdded = false;
> bool encobjAdded = false;
> + bool prmgrAdded = false;
> bool prdStarted = false;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> virJSONValuePtr secobjProps = NULL;
> virJSONValuePtr encobjProps = NULL;
> + virJSONValuePtr prmgrProps = NULL;
> qemuDomainStorageSourcePrivatePtr srcPriv;
> qemuDomainSecretInfoPtr secinfo = NULL;
> qemuDomainSecretInfoPtr encinfo = NULL;
> + qemuDomainDiskPRDPtr prd = NULL;
>
> if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0)
> goto cleanup;
> @@ -437,6 +463,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
> if (srcPriv) {
> secinfo = srcPriv->secinfo;
> encinfo = srcPriv->encinfo;
> + prd = srcPriv->prd;
As noted much earlier - the private data isn't needed here as the prd is
in the disk storage definition.
> }
>
> if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
> @@ -447,6 +474,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
> if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) <
0)
> goto error;
>
> + if (qemuMaybeBuildPRManagerInfoProps(vm, prd, &prmgrProps) < 0)
> + goto error;
> +
This should just call qemuBuildPRManagerInfoProps directly since it
already checks if !prd || !prd->alias
No, because we might end up trying to add pr-manager object that is
already there in qemu which would fail and so would whole hotplug opertaion.
Michal