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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org