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