On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> While we're not generating the command line just yet (look for
> the next commits), we can generate the alias for pr-manager.
> A domain can have up to one managed pr-manager (in which case
> socket path is decided by libvirt and pr-helper is spawned by
> libvirt too), but up to ndisks of unmanaged ones (one per disk
> basically). In case of the former we can generate a simple alias
> and be sure it'll not conflict. But in case of the latter to
Well if it's only one, then it had better not conflict (IOW: after the
and is unnecessary because there is check).
> avoid any conflicts let's generate alias that's based on
> something unique - like disk target.
>
Make sure this commit message follows whatever happens in this patch...
> Signed-off-by: Michal Privoznik <mpriv...@redhat.com>
> ---
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_alias.c | 11 ++++++
> src/qemu/qemu_alias.h | 2 ++
> src/qemu/qemu_domain.c | 87
> +++++++++++++++++++++++++++++++++++++++++++++--
> src/qemu/qemu_domain.h | 10 ++++++
> src/util/virstoragefile.c | 15 ++++++++
> src/util/virstoragefile.h | 2 ++
> 7 files changed, 126 insertions(+), 3 deletions(-)
>
This patch does two things - one is just the pure alias/path for the
pr-helper for the active domain. The second is copy that same
information to the private storage source, which I'm not sure (yet) why
it's necessary since both can be regenerated as needed based on fairly
static data as opposed to secinfo and encinfo which get filled in with
information only available during Prepare (the interaction with the
secret driver and the need for the @conn pointer) that wasn't available
during launch/command line building. Although some of that has changed
with more recent changes to be able to get a secret conn "on the fly".
Anyway, I digress.
The point being - please note why you're also saving something in the
private storage source area... The 'path' is already present in the
domain XML (both active and inactive) and the 'alias' could be saved in
the active output if you tweaked virStoragePRDefFormat to match what
virDomainDeviceInfoFormat does.
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a376e3bb0d..5b7b5514a2 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2799,7 +2799,9 @@ virStorageNetHostTransportTypeToString;
> virStorageNetProtocolTypeToString;
> virStoragePRDefFormat;
> virStoragePRDefFree;
> +virStoragePRDefIsEnabled;
> virStoragePRDefIsEqual;
> +virStoragePRDefIsManaged;
> virStoragePRDefParseXML;
> virStorageSourceBackingStoreClear;
> virStorageSourceClear;
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index 95d1e0370a..6db3da27a8 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -773,3 +773,14 @@ qemuAliasChardevFromDevAlias(const char *devAlias)
>
> return ret;
> }
> +
> +
> +char *
> +qemuDomainGetManagedPRAlias(void)
> +{
> + char *alias;
> +
> + ignore_value(VIR_STRDUP(alias, "pr-helper0"));
> +
> + return alias;
> +}
I don't really see a purpose for this function. If it were to survive,
then it could take a parameter "const char *srcalias" and create/return
the alias from that based on what's in qemuDomainPrepareDiskPRD.
Eventually when the qemuProcessKillPRDaemon is created, it doesn't
necessarily distinguish between managed and unmanaged... Still having it
fail because it couldn't strdup what amounts to be a static string
doesn't make much sense.
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
> index 8c744138ce..91e0a9c893 100644
> --- a/src/qemu/qemu_alias.h
> +++ b/src/qemu/qemu_alias.h
> @@ -92,4 +92,6 @@ char *qemuAliasTLSObjFromSrcAlias(const char *srcAlias)
> char *qemuAliasChardevFromDevAlias(const char *devAlias)
> ATTRIBUTE_NONNULL(1);
>
> +char * qemuDomainGetManagedPRAlias(void);
> +
> #endif /* __QEMU_ALIAS_H__*/
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5a7b5f8417..361a80be84 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -982,6 +982,18 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr
> *secinfo)
> }
>
>
> +static void
> +qemuDomainDiskPRDFree(qemuDomainDiskPRDPtr prd)
> +{
> + if (!prd)
> + return;
> +
> + VIR_FREE(prd->alias);
> + VIR_FREE(prd->path);
> + VIR_FREE(prd);
> +}
> +
> +
> static virClassPtr qemuDomainDiskPrivateClass;
> static void qemuDomainDiskPrivateDispose(void *obj);
>
> @@ -1062,6 +1074,7 @@ qemuDomainStorageSourcePrivateDispose(void *obj)
>
> qemuDomainSecretInfoFree(&priv->secinfo);
> qemuDomainSecretInfoFree(&priv->encinfo);
> + qemuDomainDiskPRDFree(priv->prd);
> }
>
>
> @@ -1473,9 +1486,6 @@
> qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivatePtr priv,
> if (!hasAuth && !hasEnc)
> return 0;
>
> - if (!(src->privateData = qemuDomainStorageSourcePrivateNew()))
> - return -1;
> -
> srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
>
> if (hasAuth) {
> @@ -11925,11 +11935,79 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr
> disk)
> }
>
>
> +static int
> +qemuDomainPrepareDiskPRD(qemuDomainObjPrivatePtr priv,
> + virDomainDiskDefPtr disk)
> +{
> + qemuDomainStorageSourcePrivatePtr srcPriv;
> + virStoragePRDefPtr prd = disk->src->pr;
> + char *prAlias = NULL;
> + char *prPath = NULL;
> + int ret = -1;
> +
> + if (!virStoragePRDefIsEnabled(prd))
> + return 0;
> +
> + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("reservations not supported with this QEMU
> binary"));
> + return ret;
> + }
> +
> + if (!virStorageSourceIsLocalStorage(disk->src)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("reservations supported only for local storage"));
> + return ret;
> + }
Not only local, but local and LUN ... Still this check probably should
have gone into the Validate code if anything.
> +
> + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +
> + /* Managed PR means there's one pr-manager object per domain
> + * and the pr-helper process is spawned and managed by
> + * libvirt.
> + * If PR is unmanaged there's one pr-manager object per disk
> + * (in general each disk can have its own pr-manager) and
> + * it's user's responsibility to start the pr-helper process.
> + */
> + if (virStoragePRDefIsManaged(prd)) {
> + /* Generate PR helper socket path & alias that are same
> + * for each disk in the domain. */
> +
> + if (!(prAlias = qemuDomainGetManagedPRAlias()))
just make it a straight VIR_STRDUP("pr-helper0")
or some sort of #define for VIRT_PR_MANAGED_ALIAS that's usable by
qemuProcessKillPRDaemon... You could go really obscure by using
"pr-helper" as the defined value and then using that in all your various
printf's.
John
> + return ret;
> +
> + if (virAsprintf(&prPath, "%s/pr-helper0.sock", priv->libDir) < 0)
> + goto cleanup;
> +
> + } else {
> + if (virAsprintf(&prAlias, "pr-helper-%s", disk->info.alias) < 0)
> + return ret;
> +
> + if (VIR_STRDUP(prPath, prd->path) < 0)
> + goto cleanup;
> + }
> +
> + if (VIR_ALLOC(srcPriv->prd) < 0)
> + goto cleanup;
> + VIR_STEAL_PTR(srcPriv->prd->alias, prAlias);
> + VIR_STEAL_PTR(srcPriv->prd->path, prPath);
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(prPath);
> + VIR_FREE(prAlias);
> + return ret;
> +}
> +
> +
> int
> qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
> qemuDomainObjPrivatePtr priv,
> virQEMUDriverConfigPtr cfg)
> {
> + if (!(disk->src->privateData = qemuDomainStorageSourcePrivateNew()))
> + return -1;
> +
> qemuDomainPrepareDiskCachemode(disk);
>
> if (qemuDomainPrepareDiskSourceTLS(disk->src, cfg) < 0)
> @@ -11938,6 +12016,9 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk,
> if (qemuDomainSecretDiskPrepare(priv, disk) < 0)
> return -1;
>
> + if (qemuDomainPrepareDiskPRD(priv, disk) < 0)
> + return -1;
> +
> if (qemuDomainPrepareDiskSourceChain(disk, NULL, cfg, priv->qemuCaps) <
> 0)
> return -1;
>
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 21e12f6594..d283f128ef 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -373,6 +373,13 @@ struct _qemuDomainDiskPrivate {
> bool removable; /* device media can be removed/changed */
> };
>
> +typedef struct _qemuDomainDiskPRD qemuDomainDiskPRD;
> +typedef qemuDomainDiskPRD *qemuDomainDiskPRDPtr;
> +struct _qemuDomainDiskPRD {
> + char *alias;
> + char *path; /* socket path. */
> +};
> +
> # define QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src) \
> ((qemuDomainStorageSourcePrivatePtr) (src)->privateData)
>
> @@ -386,6 +393,9 @@ struct _qemuDomainStorageSourcePrivate {
>
> /* data required for decryption of encrypted storage source */
> qemuDomainSecretInfoPtr encinfo;
> +
> + /* data required for persistent reservations */
> + qemuDomainDiskPRDPtr prd;
> };
>
> virObjectPtr qemuDomainStorageSourcePrivateNew(void);
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index b017024b2f..877d8ba8e4 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2040,6 +2040,21 @@ virStoragePRDefIsEqual(virStoragePRDefPtr a,
> return true;
> }
>
> +
> +bool
> +virStoragePRDefIsEnabled(virStoragePRDefPtr prd)
> +{
> + return prd && prd->enabled == VIR_TRISTATE_BOOL_YES;
> +}
> +
> +
> +bool
> +virStoragePRDefIsManaged(virStoragePRDefPtr prd)
> +{
> + return prd && prd->managed == VIR_TRISTATE_BOOL_YES;
> +}
> +
> +
> virSecurityDeviceLabelDefPtr
> virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
> const char *model)
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 77853eefe8..2127509ea3 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -385,6 +385,8 @@ void virStoragePRDefFormat(virBufferPtr buf,
> virStoragePRDefPtr prd);
> bool virStoragePRDefIsEqual(virStoragePRDefPtr a,
> virStoragePRDefPtr b);
> +bool virStoragePRDefIsEnabled(virStoragePRDefPtr prd);
> +bool virStoragePRDefIsManaged(virStoragePRDefPtr prd);
>
> virSecurityDeviceLabelDefPtr
> virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src,
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
