Re: [libvirt] [PATCH 04/14] qemu_domain: Introduce qemuDomainDiskPRObject

[Peter Krempa]

On Thu, Jan 18, 2018 at 17:04:36 +0100, Michal Privoznik wrote:
> This is an extended definition of virStoragePRDef because it
> contains runtime information (like path to pr helper socket, its
> pid and alias). Since these are driver dependant we should have a
> driver specific structure instead of putting all of that into
> driver agnostic structure.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 120 
> +
>  src/qemu/qemu_domain.h |  18 
>  2 files changed, 138 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e8539dcab..7fa8c93b7 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -65,6 +65,7 @@
>  #endif
>  #include 
>  #include 
> +#include 
>  #if defined(HAVE_SYS_MOUNT_H)
>  # include 
>  #endif
> @@ -1829,6 +1830,9 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr 
> priv)
>  
>  virBitmapFree(priv->migrationCaps);
>  priv->migrationCaps = NULL;
> +
> +virHashFree(priv->prHelpers);
> +priv->prHelpers = NULL;
>  }
>  
>  
> @@ -10917,6 +10921,122 @@ 
> qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
>  }
>  
>  
> +static void
> +qemuDomainDiskPRObjectHashFree(void *payload,
> +   const void *name)
> +{
> +qemuDomainDiskPRObjectPtr tmp = payload;
> +
> +if (tmp->managed &&
> +tmp->pid != (pid_t) -1) {
> +VIR_DEBUG("Forcibly killing pr-manager: %s", (const char *) name);
> +virProcessKillPainfully(tmp->pid, true);

Is this really a good idea? If you restart libvirtd gracefully, this
will kill all the PR daemons.

Also I'd prefer if all the process management code would be in
qemu_process.c.


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

[libvirt] [PATCH 04/14] qemu_domain: Introduce qemuDomainDiskPRObject

[Michal Privoznik]

This is an extended definition of virStoragePRDef because it
contains runtime information (like path to pr helper socket, its
pid and alias). Since these are driver dependant we should have a
driver specific structure instead of putting all of that into
driver agnostic structure.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 120 +
 src/qemu/qemu_domain.h |  18 
 2 files changed, 138 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e8539dcab..7fa8c93b7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -65,6 +65,7 @@
 #endif
 #include 
 #include 
+#include 
 #if defined(HAVE_SYS_MOUNT_H)
 # include 
 #endif
@@ -1829,6 +1830,9 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr 
priv)
 
 virBitmapFree(priv->migrationCaps);
 priv->migrationCaps = NULL;
+
+virHashFree(priv->prHelpers);
+priv->prHelpers = NULL;
 }
 
 
@@ -10917,6 +10921,122 @@ qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr 
driver,
 }
 
 
+static void
+qemuDomainDiskPRObjectHashFree(void *payload,
+   const void *name)
+{
+qemuDomainDiskPRObjectPtr tmp = payload;
+
+if (tmp->managed &&
+tmp->pid != (pid_t) -1) {
+VIR_DEBUG("Forcibly killing pr-manager: %s", (const char *) name);
+virProcessKillPainfully(tmp->pid, true);
+}
+VIR_FREE(tmp->path);
+VIR_FREE(tmp);
+}
+
+
+/**
+ * qemuDomainDiskPRObjectRegister:
+ * @priv: Domain private data
+ * @alias: alias of the pr-manager object
+ * @managed: true if pr-managed object is manged by libvirt
+ * @path: socket path for the pr-manager object
+ *
+ * Records [alias, managed, path] tuple for pr-manager objects.
+ * On successful return @path is stolen and set to NULL.
+ *
+ * Returns 0 on success (with @path stolen),
+ *-1 otherwise (with error reported).
+ */
+int
+qemuDomainDiskPRObjectRegister(qemuDomainObjPrivatePtr priv,
+   const char *alias,
+   bool managed,
+   char **path)
+{
+qemuDomainDiskPRObjectPtr tmp;
+int ret = -1;
+
+if (!priv->prHelpers &&
+!(priv->prHelpers = virHashCreate(10, qemuDomainDiskPRObjectHashFree)))
+return -1;
+
+if ((tmp = virHashLookup(priv->prHelpers, alias))) {
+/* Entry exists, check if it matches path. This shouldn't
+ * happen, but it's better to be safe than sorry. */
+if (STRNEQ(tmp->path, *path)) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("trying to change path for pr helper object"));
+
+return -1;
+}
+
+/* Claim success */
+VIR_FREE(*path);
+return 0;
+}
+
+if (VIR_ALLOC(tmp) < 0)
+goto cleanup;
+
+tmp->managed = managed,
+tmp->path = *path;
+tmp->pid = (pid_t) -1;
+
+if (virHashAddEntry(priv->prHelpers, alias, tmp) < 0)
+goto cleanup;
+
+*path = NULL;
+tmp = NULL;
+ret = 0;
+ cleanup:
+VIR_FREE(tmp);
+return ret;
+}
+
+
+static int
+qemuDomainDiskPRObjectKillOne(void *payload,
+  const void *name,
+  void *data ATTRIBUTE_UNUSED)
+{
+qemuDomainDiskPRObjectPtr tmp = payload;
+
+if (!tmp->managed)
+return 0;
+
+VIR_DEBUG("Killing pr-manager: %s", (const char *) name);
+if (tmp->pid != (pid_t) -1 &&
+virProcessKill(tmp->pid, SIGTERM) < 0) {
+virReportSystemError(errno,
+ _("Unable to kill pr-manager: %s"),
+ (const char *) name);
+/* Don't return error; we want to kill as many as
+ * possible. */
+} else {
+tmp->pid = (pid_t) -1;
+}
+
+return 0;
+}
+
+
+void
+qemuDomainDiskPRObjectKillAll(qemuDomainObjPrivatePtr priv)
+{
+if (!priv->prHelpers)
+return;
+
+virHashForEach(priv->prHelpers,
+   qemuDomainDiskPRObjectKillOne, NULL);
+
+virHashFree(priv->prHelpers);
+priv->prHelpers = NULL;
+}
+
+
 int
 qemuDomainPrepareDiskSource(virConnectPtr conn,
 virDomainDiskDefPtr disk,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index ddfc46dcd..f741f3039 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -329,6 +329,8 @@ struct _qemuDomainObjPrivate {
 /* Migration capabilities. Rechecked on reconnect, not to be saved in
  * private XML. */
 virBitmapPtr migrationCaps;
+
+virHashTablePtr prHelpers;
 };
 
 # define QEMU_DOMAIN_PRIVATE(vm) \
@@ -990,4 +992,20 @@ qemuDomainPrepareDiskSource(virConnectPtr conn,
 qemuDomainObjPrivatePtr priv,
 virQEMUDriverConfigPtr cfg);
 
+typedef struct _qemuDomainDiskPRObject qemuDomainDiskPRObject;
+typedef qemuDomainDiskPRObject