[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/1] lvm storage backend: handle command_names=1 in lvm.conf (v2)



On 09/28/2011 02:08 PM, Serge E. Hallyn wrote:
[ Thanks for the feedback, Eric.  Hopefully I correctly incorporated it
in this version.  This version still tests fine with and without
lvm.conf command_names=1 ]

Glad that it passed testing (as I had not tested my suggestions).


If the regexes supported (?:pvs)?, then we could handle this by
optionally matching but not returning the initial command name.  But it
doesn't.  So add a new char* argument to
virStorageBackendRunProgRegex().  If that argument is NULL then we act
as usual.  Otherwise, if the string at that argument is found at the
start of a returned line, we drop that before running the regex.

With this patch, virt-manager shows me lvs with command_names 1 or 0.

The definitions of PVS_BASE etc may want to be moved into the configure
scripts (though given how PVS is found, IIUC that could only happen if
pvs was a link to pvs_real), but in any case no sense dealing with that
until we're sure this is an ok way to handle it.

Changelog:
   Sep 28: Use STRSKIP and make cmd_to_ignore arg const, as per Eric's
           feedback.

I'll tweak the commit a bit (information like this is useful to the patch review, but less useful in 'git log' - so the best place to stick it is after the --- divider, so that 'git am' will know that it is not part of the official commit message).

@@ -1460,13 +1460,20 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool,
      }

      while (fgets(line, sizeof(line), list) != NULL) {
+        char *p = NULL;
          /* Strip trailing newline */
          int len = strlen(line);
          if (len&&  line[len-1] == '\n')
              line[len-1] = '\0';

+        /* if cmd_to_ignore is specified, then ignore it */

I'll tweak this comment slightly to mention that we are skipping a prefix, if it is present, and not ignoring the entire command line.

+++ b/src/storage/storage_backend_logical.c
@@ -205,13 +205,14 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
          pool->def->source.name, NULL
      };

+#define LVS_BASE "lvs"
      if (virStorageBackendRunProgRegex(pool,
                                        prog,
                                        1,
                                        regexes,
                                        vars,
                                        virStorageBackendLogicalMakeVol,
-                                      vol)<  0) {
+                                      vol, LVS_BASE)<  0) {

I'm not a big fan of in-function #define, especially if it's for a one-shot string literal. I generally either float the #define up to the top of the file (out of the function), or in this case, inline the string constant into its lone usage point (we can refactor into a #define later, if we need to use the string more than once).

ACK and pushed with this squashed in:

diff --git i/src/storage/storage_backend.c w/src/storage/storage_backend.c
index dd7e36b..64c35c2 100644
--- i/src/storage/storage_backend.c
+++ w/src/storage/storage_backend.c
@@ -1400,7 +1400,7 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool,
                               const char **regex,
                               int *nvars,
                               virStorageBackendListVolRegexFunc func,
-                              void *data, const char *cmd_to_ignore)
+                              void *data, const char *prefix)
 {
     int fd = -1, err, ret = -1;
     FILE *list = NULL;
@@ -1466,9 +1466,9 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool,
         if (len && line[len-1] == '\n')
             line[len-1] = '\0';

-        /* if cmd_to_ignore is specified, then ignore it */
-        if (cmd_to_ignore)
-            p = STRSKIP(line, cmd_to_ignore);
+        /* ignore any command prefix */
+        if (prefix)
+            p = STRSKIP(line, prefix);
         if (!p)
             p = line;

diff --git i/src/storage/storage_backend_logical.c w/src/storage/storage_backend_logical.c
index 7923b71..589f486 100644
--- i/src/storage/storage_backend_logical.c
+++ w/src/storage/storage_backend_logical.c
@@ -205,14 +205,13 @@ virStorageBackendLogicalFindLVs(virStoragePoolObjPtr pool,
         pool->def->source.name, NULL
     };

-#define LVS_BASE "lvs"
     if (virStorageBackendRunProgRegex(pool,
                                       prog,
                                       1,
                                       regexes,
                                       vars,
                                       virStorageBackendLogicalMakeVol,
-                                      vol, LVS_BASE) < 0) {
+                                      vol, "lvs") < 0) {
         return -1;
     }

@@ -328,10 +327,9 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED,
     memset(&sourceList, 0, sizeof(sourceList));
     sourceList.type = VIR_STORAGE_POOL_LOGICAL;

-#define PVS_BASE "pvs"
     if (virStorageBackendRunProgRegex(NULL, prog, 1, regexes, vars,

virStorageBackendLogicalFindPoolSourcesFunc,
-                                &sourceList, PVS_BASE) < 0)
+                                &sourceList, "pvs") < 0)
         return NULL;

     retval = virStoragePoolSourceListFormat(&sourceList);
@@ -498,7 +496,6 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
         return -1;
     }

-#define VGS_BASE "vgs"
     /* Now get basic volgrp metadata */
     if (virStorageBackendRunProgRegex(pool,
                                       prog,
@@ -506,7 +503,7 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED,
                                       regexes,
                                       vars,

virStorageBackendLogicalRefreshPoolFunc,
-                                      NULL, VGS_BASE) < 0) {
+                                      NULL, "vgs") < 0) {
         virStoragePoolObjClearVols(pool);
         return -1;
     }


--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]