On 02/21/2018 01:11 PM, 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
> avoid any conflicts let's generate alias that's based on
> something unique - like disk target.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++++++--
> src/qemu/qemu_domain.h | 10 ++++++
> src/util/virstoragefile.c | 15 +++++++++
> src/util/virstoragefile.h | 2 ++
> 5 files changed, 106 insertions(+), 3 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d4bff0f0b..bb0183ea6 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2781,7 +2781,9 @@ virStorageNetHostTransportTypeToString;
> virStorageNetProtocolTypeToString;
> virStoragePRDefFormat;
> virStoragePRDefFree;
> +virStoragePRDefIsEnabled;
> virStoragePRDefIsEqual;
> +virStoragePRDefIsManaged;
> virStoragePRDefParseNode;
> virStorageSourceBackingStoreClear;
> virStorageSourceClear;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index de8974d66..dffc4c30e 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;
> -
How does the following not fail with the previous hunk removed? Oh, I
see, it's moved to qemuDomainPrepareDiskSource.... could be separated
out, but I see why it's here - so fine. Just eye opening ;-)
> srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src);
>
> if (hasAuth) {
> @@ -11538,17 +11548,81 @@ qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
> }
>
>
> +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"));
> + goto cleanup;
> + }
> +
> + if (!virStorageSourceIsLocalStorage(disk->src)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("reservations supported only for local storage"));
> + goto cleanup;
> + }
Ironically the above two could return -1 directly...
> +
> + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +
> + if (virStoragePRDefIsManaged(prd)) {
> + /* Generate PR helper socket path & alias that are same
> + * for each disk in the domain. */
> +
> + if (VIR_STRDUP(prAlias, "pr-helper0") < 0)
> + return -1;
> +
> + if (virAsprintf(&prPath, "%s/pr-helper0.sock", priv->libDir) < 0)
> + return -1;
Leaks prAlias
BTW: Whatever "managed" ends up being needs to be described somewhere
either prior to this or after this, but knowing how the connection
between libvirt and qemu's process is going to happen will be really
helpful... Something that I would think would be described in patch 1,
but again that's just me. This whole managed thing is really confusing
the way it's written. May make sense to you, but it doesn't make sense
to me, just saying.
> +
> + } else {
> + if (virAsprintf(&prAlias, "pr-helper-%s", disk->dst) < 0)
> + return -1;
Or using the 'disk->info.alias' like other consumers.
> +
> + if (VIR_STRDUP(prPath, prd->path) < 0)
> + return -1;
Leaks prAlias
I'm going to stop here - mainly because I'm confused about managed, but
hopefully a fresh look in the morning will help.
John
> + }
> +
> + 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;
> +
> if (qemuDomainPrepareDiskSourceTLS(disk->src, cfg) < 0)
> return -1;
>
> if (qemuDomainSecretDiskPrepare(priv, disk) < 0)
> return -1;
>
> + if (qemuDomainPrepareDiskPRD(priv, disk) < 0)
> + return -1;
> +
> if (disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
> disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER &&
> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_GLUSTER_DEBUG_LEVEL)) {
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 6d3e6eb5e..b9258eb8e 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 328f59788..a85fec330 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2076,6 +2076,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 e70c99076..cf931bbb1 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
