Re: [libvirt] [PATCH 05/14] qemu: Generate alias for pr-helper

[Peter Krempa]

On Thu, Jan 18, 2018 at 17:04:37 +0100, Michal Privoznik wrote:
> While we're not generating the command line just yet (look for
> the next commit), 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 
> ---
>  src/qemu/qemu_domain.c | 61 
> ++
>  src/qemu/qemu_domain.h |  3 +++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 7fa8c93b7..e8d2adf56 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -972,6 +972,8 @@ qemuDomainStorageSourcePrivateDispose(void *obj)
>  
>  qemuDomainSecretInfoFree(>secinfo);
>  qemuDomainSecretInfoFree(>encinfo);
> +
> +VIR_FREE(priv->prAlias);
>  }
>  
>  
> @@ -11037,6 +11039,62 @@ 
> qemuDomainDiskPRObjectKillAll(qemuDomainObjPrivatePtr priv)
>  }
>  
>  
> +static int
> +qemuDomainPrepareDiskPR(qemuDomainObjPrivatePtr priv,
> +virDomainDiskDefPtr disk)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv;
> +virStoragePRDefPtr prd = disk->src->pr;
> +char *prPath = NULL;
> +bool managed;
> +int ret = -1;
> +
> +if (!prd ||
> +prd->enabled != VIR_TRISTATE_BOOL_YES)
> +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 (!disk->src->privateData &&
> +!(disk->src->privateData = qemuDomainStorageSourcePrivateNew()))
> +return -1;
> +
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +managed = prd->managed == VIR_TRISTATE_BOOL_YES;
> +
> +if (managed) {
> +/* Generate PR helper socket path & alias that are same
> + * for each disk in the domain. */
> +
> +if (VIR_STRDUP(srcPriv->prAlias, "pr-helper0") < 0)
> +return -1;
> +
> +if (virAsprintf(, "%s/pr-helper0.sock", priv->libDir) < 0)
> +return -1;
> +
> +} else {
> +if (virAsprintf(>prAlias, "pr-helper-%s", disk->dst) < 0)
> +return -1;


I though that unmanaged PRs would be shared which would make sense given
the data structure you've introduced in patch 4. Since this code would
in that case make problems with hotplug I looked back at that code.

So, this means that there's exactly one managed PR daemon and every
unmanaged PR daemon has possibly multiple instances/connections from
qemu.

So, that brings me to the question: Do you really need the prHelpers
hash table? If the instances are duplicated, you can store the data in
virStorageSource (in the private data) and don't really need the table.

If there's intent to add sharing of the pr objects in qemu, you'll need
to rething this code, since generating the alias will create problems
when you hotunplug a shared PR instance and try to plug back a disk with
a different one.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 05/14] qemu: Generate alias for pr-helper

[Michal Privoznik]

While we're not generating the command line just yet (look for
the next commit), 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 
---
 src/qemu/qemu_domain.c | 61 ++
 src/qemu/qemu_domain.h |  3 +++
 2 files changed, 64 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 7fa8c93b7..e8d2adf56 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -972,6 +972,8 @@ qemuDomainStorageSourcePrivateDispose(void *obj)
 
 qemuDomainSecretInfoFree(>secinfo);
 qemuDomainSecretInfoFree(>encinfo);
+
+VIR_FREE(priv->prAlias);
 }
 
 
@@ -11037,6 +11039,62 @@ qemuDomainDiskPRObjectKillAll(qemuDomainObjPrivatePtr 
priv)
 }
 
 
+static int
+qemuDomainPrepareDiskPR(qemuDomainObjPrivatePtr priv,
+virDomainDiskDefPtr disk)
+{
+qemuDomainStorageSourcePrivatePtr srcPriv;
+virStoragePRDefPtr prd = disk->src->pr;
+char *prPath = NULL;
+bool managed;
+int ret = -1;
+
+if (!prd ||
+prd->enabled != VIR_TRISTATE_BOOL_YES)
+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 (!disk->src->privateData &&
+!(disk->src->privateData = qemuDomainStorageSourcePrivateNew()))
+return -1;
+
+srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
+managed = prd->managed == VIR_TRISTATE_BOOL_YES;
+
+if (managed) {
+/* Generate PR helper socket path & alias that are same
+ * for each disk in the domain. */
+
+if (VIR_STRDUP(srcPriv->prAlias, "pr-helper0") < 0)
+return -1;
+
+if (virAsprintf(, "%s/pr-helper0.sock", priv->libDir) < 0)
+return -1;
+
+} else {
+if (virAsprintf(>prAlias, "pr-helper-%s", disk->dst) < 0)
+return -1;
+
+if (VIR_STRDUP(prPath, prd->path) < 0)
+return -1;
+}
+
+if (qemuDomainDiskPRObjectRegister(priv, srcPriv->prAlias,
+   managed, ) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+VIR_FREE(prPath);
+return ret;
+}
+
+
 int
 qemuDomainPrepareDiskSource(virConnectPtr conn,
 virDomainDiskDefPtr disk,
@@ -11049,6 +11107,9 @@ qemuDomainPrepareDiskSource(virConnectPtr conn,
 if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0)
 return -1;
 
+if (qemuDomainPrepareDiskPR(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 f741f3039..5fdb5b931 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -375,6 +375,9 @@ struct _qemuDomainStorageSourcePrivate {
 
 /* data required for decryption of encrypted storage source */
 qemuDomainSecretInfoPtr encinfo;
+
+/* alias for pr-manager-helper */
+char *prAlias;
 };
 
 virObjectPtr qemuDomainStorageSourcePrivateNew(void);
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list