On 02/21/2018 01:11 PM, Michal Privoznik wrote:
> Just line in previous commit, if we are the last ones that are
> using the pr-manager delete it and also kill the pr-helper
> process.
>
> Signed-off-by: Michal Privoznik <mpriv...@redhat.com>
> ---
> src/qemu/qemu_domain.c | 19 +++++++++++++++++++
> src/qemu/qemu_domain.h | 3 +++
> src/qemu/qemu_hotplug.c | 22 ++++++++++++++++++++++
> 3 files changed, 44 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f7da62dba..acfa1d88c 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11650,6 +11650,25 @@
> qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
> }
>
>
> +size_t
> +qemuDomainGetPRDManagedCount(const virDomainDef *def)
> +{
> + size_t i;
> + size_t nmanaged = 0;
> +
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainDiskDefPtr disk = def->disks[i];
> +
> + if (!virStoragePRDefIsManaged(disk->src->pr))
> + continue;
> +
> + nmanaged++;
> + }
> +
> + return nmanaged;
> +}
> +
> +
The above feels like it could have gone in much earlier and could have
been useful for the qemuProcessKillPRDaemons changes.
> static int
> qemuDomainPrepareDiskPRD(qemuDomainObjPrivatePtr priv,
> virDomainDiskDefPtr disk)
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 418b47153..c2f67f379 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -1007,6 +1007,9 @@ qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr
> driver,
> virDomainObjPtr vm,
> qemuDomainAsyncJob asyncJob);
>
> +size_t
> +qemuDomainGetPRDManagedCount(const virDomainDef *def);
> +
> int
> qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
> qemuDomainObjPrivatePtr priv,
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 2ebb68fbc..d8460cdb4 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -400,6 +400,13 @@ qemuDestroyPRDefObject(virDomainObjPtr vm,
> }
>
>
> +static bool
> +qemuDomainDiskNeedRemovePR(virDomainObjPtr vm)
> +{
> + return qemuDomainGetPRDManagedCount(vm->def) == 1;
> +}
> +
> +> /**
> * qemuDomainAttachDiskGeneric:
> *
> @@ -3812,6 +3819,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
> char *drivestr;
> char *objAlias = NULL;
> char *encAlias = NULL;
> + const char *prAlias = NULL;
> + const char *prPath = NULL;
>
> VIR_DEBUG("Removing disk %s from domain %p %s",
> disk->info.alias, vm, vm->def->name);
> @@ -3849,6 +3858,14 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
> }
> }
>
> + if (qemuDomainDiskNeedRemovePR(vm)) {
> + qemuDomainStorageSourcePrivatePtr srcPriv;
> +
> + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> + prAlias = srcPriv->prd->alias;
> + prPath = srcPriv->prd->path;
> + }
> +
As noted in previous patch - should we really care? It's mostly a
thrashing concern, but here you don't really even remove the object if
this isn't the last one using PR.
> qemuDomainObjEnterMonitor(driver, vm);
>
> qemuMonitorDriveDel(priv->mon, drivestr);
> @@ -3864,6 +3881,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
> ignore_value(qemuMonitorDelObject(priv->mon, encAlias));
> VIR_FREE(encAlias);
>
> + if (prAlias)
> + ignore_value(qemuMonitorDelObject(priv->mon, prAlias));
> +
Since you only set prAlias when you'll be needing to remove the PR, thus
if there was more than one lun w/ PR, you wouldn't delete the object.
> if (disk->src->haveTLS)
> ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias));
>
> @@ -3882,6 +3902,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
> }
> }
>
> + qemuDestroyPRDefObject(vm, prAlias, prPath);
> +
If the two patches were combined it'd be more easier to see, but this
would also not unlink the socket path... If there were 2 luns using PR
and just 1 was removed, we wouldn't delete the object and prAlias would
still be NULL meaning qemuDestroyPRDefObject returns immediately and
nothing really happens.
Again, the whole socket path and managed vs. unmanaged is not very clear
at least to me.
John
> qemuDomainReleaseDeviceAddress(vm, &disk->info, src);
>
> if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
