[libvirt PATCH 04/17] util: move virStorageFileGetLVMKey to locking

Pavel Hrdina posted 17 patches 5 years, 1 month ago
[libvirt PATCH 04/17] util: move virStorageFileGetLVMKey to locking
Posted by Pavel Hrdina 5 years, 1 month ago
The function doesn't take virStorageSource as argument and has nothing
in common with virStorageSource or storage file.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/libvirt_private.syms        |  1 -
 src/locking/lock_driver_lockd.c | 66 ++++++++++++++++++++++++++++++++-
 src/util/virstoragefile.c       | 62 -------------------------------
 src/util/virstoragefile.h       |  2 -
 4 files changed, 65 insertions(+), 66 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index aecc5cd654..d04b8f5e0c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3134,7 +3134,6 @@ virStorageFileFeatureTypeToString;
 virStorageFileFormatTypeFromString;
 virStorageFileFormatTypeToString;
 virStorageFileGetBackingStoreStr;
-virStorageFileGetLVMKey;
 virStorageFileGetMetadata;
 virStorageFileGetMetadataFromBuf;
 virStorageFileGetMetadataFromFD;
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 15d9e5f076..b8c2121e78 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -24,6 +24,7 @@
 #include "lock_driver.h"
 #include "virconf.h"
 #include "viralloc.h"
+#include "vircommand.h"
 #include "vircrypto.h"
 #include "virlog.h"
 #include "viruuid.h"
@@ -455,6 +456,69 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock,
 }
 
 
+#ifdef LVS
+static int virLockManagerGetLVMKey(const char *path,
+                                   char **key)
+{
+    /*
+     *  # lvs --noheadings --unbuffered --nosuffix --options "uuid" LVNAME
+     *    06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky
+     */
+    int status;
+    int ret = -1;
+    g_autoptr(virCommand) cmd = NULL;
+
+    cmd = virCommandNewArgList(LVS, "--noheadings",
+                               "--unbuffered", "--nosuffix",
+                               "--options", "uuid", path,
+                               NULL
+                               );
+    *key = NULL;
+
+    /* Run the program and capture its output */
+    virCommandSetOutputBuffer(cmd, key);
+    if (virCommandRun(cmd, &status) < 0)
+        goto cleanup;
+
+    /* Explicitly check status == 0, rather than passing NULL
+     * to virCommandRun because we don't want to raise an actual
+     * error in this scenario, just return a NULL key.
+     */
+
+    if (status == 0 && *key) {
+        char *nl;
+        char *tmp = *key;
+
+        /* Find first non-space character */
+        while (*tmp && g_ascii_isspace(*tmp))
+            tmp++;
+        /* Kill leading spaces */
+        if (tmp != *key)
+            memmove(*key, tmp, strlen(tmp)+1);
+
+        /* Kill trailing newline */
+        if ((nl = strchr(*key, '\n')))
+            *nl = '\0';
+    }
+
+    ret = 0;
+
+ cleanup:
+    if (*key && STREQ(*key, ""))
+        VIR_FREE(*key);
+
+    return ret;
+}
+#else
+static int virLockManagerGetLVMKey(const char *path,
+                                   char **key G_GNUC_UNUSED)
+{
+    virReportSystemError(ENOSYS, _("Unable to get LVM key for %s"), path);
+    return -1;
+}
+#endif
+
+
 static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
                                                unsigned int type,
                                                const char *name,
@@ -494,7 +558,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
         if (STRPREFIX(name, "/dev") &&
             driver->lvmLockSpaceDir) {
             VIR_DEBUG("Trying to find an LVM UUID for %s", name);
-            if (virStorageFileGetLVMKey(name, &newName) < 0)
+            if (virLockManagerGetLVMKey(name, &newName) < 0)
                 goto cleanup;
 
             if (newName) {
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 119342d296..bc342cabe3 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1251,68 +1251,6 @@ int virStorageFileIsClusterFS(const char *path)
                                  VIR_FILE_SHFS_CEPH);
 }
 
-#ifdef LVS
-int virStorageFileGetLVMKey(const char *path,
-                            char **key)
-{
-    /*
-     *  # lvs --noheadings --unbuffered --nosuffix --options "uuid" LVNAME
-     *    06UgP5-2rhb-w3Bo-3mdR-WeoL-pytO-SAa2ky
-     */
-    int status;
-    int ret = -1;
-    g_autoptr(virCommand) cmd = NULL;
-
-    cmd = virCommandNewArgList(LVS, "--noheadings",
-                               "--unbuffered", "--nosuffix",
-                               "--options", "uuid", path,
-                               NULL
-                               );
-    *key = NULL;
-
-    /* Run the program and capture its output */
-    virCommandSetOutputBuffer(cmd, key);
-    if (virCommandRun(cmd, &status) < 0)
-        goto cleanup;
-
-    /* Explicitly check status == 0, rather than passing NULL
-     * to virCommandRun because we don't want to raise an actual
-     * error in this scenario, just return a NULL key.
-     */
-
-    if (status == 0 && *key) {
-        char *nl;
-        char *tmp = *key;
-
-        /* Find first non-space character */
-        while (*tmp && g_ascii_isspace(*tmp))
-            tmp++;
-        /* Kill leading spaces */
-        if (tmp != *key)
-            memmove(*key, tmp, strlen(tmp)+1);
-
-        /* Kill trailing newline */
-        if ((nl = strchr(*key, '\n')))
-            *nl = '\0';
-    }
-
-    ret = 0;
-
- cleanup:
-    if (*key && STREQ(*key, ""))
-        VIR_FREE(*key);
-
-    return ret;
-}
-#else
-int virStorageFileGetLVMKey(const char *path,
-                            char **key G_GNUC_UNUSED)
-{
-    virReportSystemError(ENOSYS, _("Unable to get LVM key for %s"), path);
-    return -1;
-}
-#endif
-
 #ifdef WITH_UDEV
 /* virStorageFileGetSCSIKey
  * @path: Path to the SCSI device
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index cc9bd4875f..31feb22f26 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -428,8 +428,6 @@ int virStorageFileIsClusterFS(const char *path);
 bool virStorageIsFile(const char *path);
 bool virStorageIsRelative(const char *backing);
 
-int virStorageFileGetLVMKey(const char *path,
-                            char **key);
 int virStorageFileGetSCSIKey(const char *path,
                              char **key,
                              bool ignoreError);
-- 
2.28.0

Re: [libvirt PATCH 04/17] util: move virStorageFileGetLVMKey to locking
Posted by Peter Krempa 5 years, 1 month ago
On Mon, Dec 14, 2020 at 16:55:24 +0100, Pavel Hrdina wrote:
> The function doesn't take virStorageSource as argument and has nothing
> in common with virStorageSource or storage file.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/libvirt_private.syms        |  1 -
>  src/locking/lock_driver_lockd.c | 66 ++++++++++++++++++++++++++++++++-
>  src/util/virstoragefile.c       | 62 -------------------------------
>  src/util/virstoragefile.h       |  2 -
>  4 files changed, 65 insertions(+), 66 deletions(-)

[...]

> +#ifdef LVS
> +static int virLockManagerGetLVMKey(const char *path,
> +                                   char **key)
> +{

Preferably convert the header to the modern function header style where
the function name starts on a new line after the type. (also in the
#else section)

> +    /*
> +     *  # lvs --noheadings --unbuffered --nosuffix --options "uuid" LVNAME

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Re: [libvirt PATCH 04/17] util: move virStorageFileGetLVMKey to locking
Posted by Pavel Hrdina 5 years, 1 month ago
On Mon, Jan 04, 2021 at 04:30:03PM +0100, Peter Krempa wrote:
> On Mon, Dec 14, 2020 at 16:55:24 +0100, Pavel Hrdina wrote:
> > The function doesn't take virStorageSource as argument and has nothing
> > in common with virStorageSource or storage file.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  src/libvirt_private.syms        |  1 -
> >  src/locking/lock_driver_lockd.c | 66 ++++++++++++++++++++++++++++++++-
> >  src/util/virstoragefile.c       | 62 -------------------------------
> >  src/util/virstoragefile.h       |  2 -
> >  4 files changed, 65 insertions(+), 66 deletions(-)
> 
> [...]
> 
> > +#ifdef LVS
> > +static int virLockManagerGetLVMKey(const char *path,
> > +                                   char **key)
> > +{
> 
> Preferably convert the header to the modern function header style where
> the function name starts on a new line after the type. (also in the
> #else section)

I opted to the old style because the majority of that file uses but I
guess there is no harm of using the modern style. I'll fix it before
pushing.

> > +    /*
> > +     *  # lvs --noheadings --unbuffered --nosuffix --options "uuid" LVNAME
> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>