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@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(-)

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;
+}
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;
+    }
+
+    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()))
+            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,
-- 
2.16.1

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

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 <mprivozn@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

On 04/13/2018 10:51 PM, John Ferlan wrote:
> 
> 
> 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 <mprivozn@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.

Okay. I will put it in the commit message. I thought we've discussed
this earlier. I rather put and info into status XML even though it might
look useless right now than having to write some crazy backcompat code
later.

> 
> 
>> 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.

The purpose is to generate alias at only one place instead of several
virAsprintf-s scattered all around the code.

> 
> Eventually when the qemuProcessKillPRDaemon is created, it doesn't
> necessarily distinguish between managed and unmanaged...

Well, unmanaged helper should not have its socket in libvirt private
path. Under any circumstances. And what qemuProcessKillPRDaemon does is:
1) it constructs PID file path
2) and kills any pr-helper that might had been left behind.

I find this approach more robust than plain "check if there is a disk
with pr-helper and if so kill it" because if libvirt is ever split brained

> Still having it
> fail because it couldn't strdup what amounts to be a static string
> doesn't make much sense.

Well, there are couple of reasons for stdup()-ing a static string:
a) it simplifies the code, e.g. qemuDomainDiskPRDFree() doesn't have to
care if alias is a static string or dynamically allocated one,
b) higher memory consumption (by 11 bytes btw) is only for a short
period of time (when constructing PID file path). Although, the alias is
kept around for entire time domain is running.

And if libvirt is unable to allocate 11 bytes then you're in much bigger
trouble anyways.
> 
>  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.

I don't think it can go there. For IsLocalStorage() to return valid
results virDomainDiskTranslateSourcePool() needs to be called first. And
it is done so only when starting domain. Not for Validate() callback.

And technically, at this point whether disk is LUN or not is checked in
virDomainDiskDefValidate(). So the control would not even get here. But
okay, I will change the error message.

> 
>> +
>> +    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")

No. Having alias generation at single location is very advantageous IMO.
Libvirt's full of small wrappers over functions. And it this case it
even makes sense.

> 
> 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.

Then I'd have to modify qemuDomainDiskPRDFree() so that for instance it
does:

  if (STRNEQ(prd->alias, QEMU_PR_MANAGER_ALIAS))
      VIR_FREE(prd->alias);
  VIR_FREE(prd->path);
  ...

which I don't quite like (to say the least).

Michal

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

On 04/16/2018 10:56 AM, Michal Privoznik wrote:
> On 04/13/2018 10:51 PM, John Ferlan wrote:
>>
>>
>> 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 <mprivozn@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.
> 
> Okay. I will put it in the commit message. I thought we've discussed
> this earlier. I rather put and info into status XML even though it might
> look useless right now than having to write some crazy backcompat code
> later.
> 

I don't think this is the same as @channelTargetDir (as you note in the
next response). The problem there was a reliance on a name that ended up
being too long and yes, it was a mess. I dunno - I think I've tried the
future proofing things too and was told it was unnecessary.

Still in order to "provide insight" into my why... Here you have a @path
that's essentially static outside of the commonly used priv->libDir and
an @alias that is static when managed. When unmanaged, the @path is
provided by a consumer and the @alias is essentially a static prefix
followed by a commonly used source destination alias. So nothing so far
that would need to be saved in two places.

Since @alias is an "internal" libvirt->qemu construct we wouldn't have
the same problem as some "external" construct changing our definition.
Unlike perhaps _qemuDomainVcpuPrivate where the alias perhaps changes
based on ordering, insert, remove, etc. (I didn't look, but made the
assumption). For PR there is one and it's not going to change. Even if
it did, we would always know the former format to search on before using
a newer format.

In the long run, since there would be only one the PR alias belongs in
the _qemuDomainObjPrivate further removing the need to have anything
private.

As noted somewhere during my review the existing need for a private
structure for secrets could now be reworked/removed because the code now
can generate the @conn on the fly rather than needing the @conn from the
original connection in order to lookup the secret. Depends on if we want
an error during command line generation or during the prepare phase.

>>
>>
>>> 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.
> 
> The purpose is to generate alias at only one place instead of several
> virAsprintf-s scattered all around the code.
> 

ug, sure this is similar to qemuDomainGetMasterKeyAlias or
qemuAssignDeviceWatchdogAlias. Since there can only ever be one managed
PR per domain, then scattered is I think limited to two places - start
and stop.

>>
>> Eventually when the qemuProcessKillPRDaemon is created, it doesn't
>> necessarily distinguish between managed and unmanaged...
> 
> Well, unmanaged helper should not have its socket in libvirt private
> path. Under any circumstances.

The KillPRDaemon doesn't distinguish between managed/unmanaged, but
callers do - perhaps wasn't as clear yet when I noted that...

> And what qemuProcessKillPRDaemon does is:
> 1) it constructs PID file path
> 2) and kills any pr-helper that might had been left behind.
> 
> I find this approach more robust than plain "check if there is a disk
> with pr-helper and if so kill it" because if libvirt is ever split brained
> 

Jumping forward 6 patches. Maybe it would have helped to have all the
logic that would create a PR process "in place". Then add the disk
logic. The two concepts are intertwined and related, but it is possible
to separate too.

>> Still having it
>> fail because it couldn't strdup what amounts to be a static string
>> doesn't make much sense.
> 
> Well, there are couple of reasons for stdup()-ing a static string:
> a) it simplifies the code, e.g. qemuDomainDiskPRDFree() doesn't have to
> care if alias is a static string or dynamically allocated one,

I don't think I was advocating usage of a static string to be saved in
the ->alias field. I was merely commenting on the need to have a helper
to perform the strdup of a static string that could be some #define
somewhere.... IOW, later on it's:

    if (VIR_STRDUP(prAlias, VIRT_PR_MANAGED_ALIAS) < 0)

instead of

    if (!(prAlias = qemuDomainGetManagedPRAlias()))

> b) higher memory consumption (by 11 bytes btw) is only for a short
> period of time (when constructing PID file path). Although, the alias is
> kept around for entire time domain is running.
> 
> And if libvirt is unable to allocate 11 bytes then you're in much bigger
> trouble anyways.

11 here, 11 there, it all adds up. But no, it's not the 11 or probably
more like 16, 32, 64, etc. whatever the underlying code would consider
it's smallest hunk.

Still if you had 10 managed LUNs, that's a lot of replicated data.

>>
>>  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.
> 
> I don't think it can go there. For IsLocalStorage() to return valid
> results virDomainDiskTranslateSourcePool() needs to be called first. And
> it is done so only when starting domain. Not for Validate() callback.

ah yes, the volume lun conundrum, sigh.


John

> 
> And technically, at this point whether disk is LUN or not is checked in
> virDomainDiskDefValidate(). So the control would not even get here. But
> okay, I will change the error message.
> 
>>
>>> +
>>> +    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")
> 
> No. Having alias generation at single location is very advantageous IMO.
> Libvirt's full of small wrappers over functions. And it this case it
> even makes sense.
> 
>>
>> 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.
> 
> Then I'd have to modify qemuDomainDiskPRDFree() so that for instance it
> does:
> 
>   if (STRNEQ(prd->alias, QEMU_PR_MANAGER_ALIAS))
>       VIR_FREE(prd->alias);
>   VIR_FREE(prd->path);
>   ...
> 
> which I don't quite like (to say the least).
> 
> Michal
> 

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

On 04/17/2018 01:49 PM, John Ferlan wrote:
> 
> 
> On 04/16/2018 10:56 AM, Michal Privoznik wrote:
>> On 04/13/2018 10:51 PM, John Ferlan wrote:
>>>
>>>
>>> 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 <mprivozn@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.
>>
>> Okay. I will put it in the commit message. I thought we've discussed
>> this earlier. I rather put and info into status XML even though it might
>> look useless right now than having to write some crazy backcompat code
>> later.
>>
> 
> I don't think this is the same as @channelTargetDir (as you note in the
> next response). The problem there was a reliance on a name that ended up
> being too long and yes, it was a mess. I dunno - I think I've tried the
> future proofing things too and was told it was unnecessary.

Depends on problem you were trying to future proof from.

> 
> Still in order to "provide insight" into my why... Here you have a @path
> that's essentially static outside of the commonly used priv->libDir and
> an @alias that is static when managed. When unmanaged, the @path is
> provided by a consumer and the @alias is essentially a static prefix
> followed by a commonly used source destination alias. So nothing so far
> that would need to be saved in two places.

Not true. If alias were a static string, then _qemuDomainDiskPRD would
need to reflect that. That means, alias must be 'const char *' because
storing a static string into 'char *' is a big NO NO. But at the same
time, alias is dynamically generated (for unmanaged disks), so it has to
be 'char *'.  Sure, we can obfuscate it by inventing new struct member,
but
just look at how ugly it looks:

  struct qemuDomainDiskPRD {
    char *alias;
    const char *constAlias;
    char *path;
  };

Also, think of all those special casing in the code that uses the
struct. This approach that I implemented here is used widely across our
code base: qemuAssignDeviceTPMAlias(), qemuAssignDeviceWatchdogAlias()
to name few.

Why we even need this struct you ask? Couple of reasons:

a) this approach implemented here is specific to QEMU and QEMU only. It
isn't hypervisor agnostic, thus it doesn't belong to _virStoragePRDef.

b) we should separate runtime and config data. That's why virStorage*()
functions live under src/util/ as they work with config and not runtime
(as in hypervisor specific). This merely mimics vm->privateData
abstraction.

> 
> Since @alias is an "internal" libvirt->qemu construct we wouldn't have
> the same problem as some "external" construct changing our definition.
> Unlike perhaps _qemuDomainVcpuPrivate where the alias perhaps changes
> based on ordering, insert, remove, etc. (I didn't look, but made the
> assumption). For PR there is one and it's not going to change. Even if
> it did, we would always know the former format to search on before using
> a newer format.

Why? We have an option to record alias used so that when libvirtd
restarts we can read it back. On the other hand what you are proposing
is to try current version, if that fails try all the previous. What for?
to save couple of bytes of memory/disk space? I don't see added value
there, sorry.

Also, the whole point of status XML is so that we don't have to use
try-error approach. Look at it from different angle: all devices have
their alias generated from info contained in config XML (either based on
their position like deviceN for N-th device, or based on their address
like scsi-0-0-0-1 for corresponding SCSI address). We can regenerate
them when reconnecting to qemu. Even better - we can ask on qemu
monitor. So why not do that and waste memory/disk space by storing them
in status XML?

> 
> In the long run, since there would be only one the PR alias belongs in
> the _qemuDomainObjPrivate further removing the need to have anything
> private.
> 
> As noted somewhere during my review the existing need for a private
> structure for secrets could now be reworked/removed because the code now
> can generate the @conn on the fly rather than needing the @conn from the
> original connection in order to lookup the secret. Depends on if we want
> an error during command line generation or during the prepare phase.
> 
>>>
>>>
>>>> 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.
>>
>> The purpose is to generate alias at only one place instead of several
>> virAsprintf-s scattered all around the code.
>>
> 
> ug, sure this is similar to qemuDomainGetMasterKeyAlias or
> qemuAssignDeviceWatchdogAlias. Since there can only ever be one managed
> PR per domain, then scattered is I think limited to two places - start
> and stop.

So? It is still better to have it at one place and called from multiple.

> 
>>>
>>> Eventually when the qemuProcessKillPRDaemon is created, it doesn't
>>> necessarily distinguish between managed and unmanaged...
>>
>> Well, unmanaged helper should not have its socket in libvirt private
>> path. Under any circumstances.
> 
> The KillPRDaemon doesn't distinguish between managed/unmanaged, but
> callers do - perhaps wasn't as clear yet when I noted that...
> 
>> And what qemuProcessKillPRDaemon does is:
>> 1) it constructs PID file path
>> 2) and kills any pr-helper that might had been left behind.
>>
>> I find this approach more robust than plain "check if there is a disk
>> with pr-helper and if so kill it" because if libvirt is ever split brained
>>
> 
> Jumping forward 6 patches. Maybe it would have helped to have all the
> logic that would create a PR process "in place". Then add the disk
> logic. The two concepts are intertwined and related, but it is possible
> to separate too.

I guess it makes perfect sense as it is now.

> 
>>> Still having it
>>> fail because it couldn't strdup what amounts to be a static string
>>> doesn't make much sense.
>>
>> Well, there are couple of reasons for stdup()-ing a static string:
>> a) it simplifies the code, e.g. qemuDomainDiskPRDFree() doesn't have to
>> care if alias is a static string or dynamically allocated one,
> 
> I don't think I was advocating usage of a static string to be saved in
> the ->alias field. I was merely commenting on the need to have a helper
> to perform the strdup of a static string that could be some #define
> somewhere.... IOW, later on it's:
> 
>     if (VIR_STRDUP(prAlias, VIRT_PR_MANAGED_ALIAS) < 0)
> 
> instead of
> 
>     if (!(prAlias = qemuDomainGetManagedPRAlias()))
> 
>> b) higher memory consumption (by 11 bytes btw) is only for a short
>> period of time (when constructing PID file path). Although, the alias is
>> kept around for entire time domain is running.
>>
>> And if libvirt is unable to allocate 11 bytes then you're in much bigger
>> trouble anyways.
> 
> 11 here, 11 there, it all adds up. But no, it's not the 11 or probably
> more like 16, 32, 64, etc. whatever the underlying code would consider
> it's smallest hunk.
> 
> Still if you had 10 managed LUNs, that's a lot of replicated data.

Read my reply above. Obfuscating code just to save couple of bytes of
memory is not a good idea in my book.

Michal

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