[libvirt] [Patch 0/3]virsh: Patches for virsh logging
Virsh logging has some basic issues: 1. In code, magic numbers are used for logging rather than loglevel variables. 2. Magic number "5" is used for logging which doesn't map to any loglevel variable. Valid loglevel range is 0-4 3. Usage of loglevel variables doesn't align with that of libvirt logging. In libvirt "DEBUG" loglevel is the superset and logs messages at all other levels. Whereas in virsh, "ERROR" loglevel behaves this way, which needs correction 4. virsh man page and code are inconsistent with respect to loglevels Following patchset is to address the above mentioned issues. 1/3 - Avoid using magic numbers for logging 2/3 - Align log level usage to that of libvirt 3/3 - Update virsh manpage with related changes tools/virsh.pod | 30 ++++++++++++ tools/virsh.c | 124 +++++++++++++++++++++++++++--------------------- 2 files changed, 102 insertions(+), 52 deletions(-)
Replace magic numbers with loglevel variables. Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> --- tools/virsh.c | 112 +++++++++++++++++++++++++++++++++------------------------- 1 file changed, 65 insertions(+), 47 deletions(-) Index: libvirt/tools/virsh.c =================================================================== --- libvirt.orig/tools/virsh.c +++ libvirt/tools/virsh.c @@ -2376,7 +2376,8 @@ cmdDominfo(vshControl *ctl, const vshCmd /* Check and display whether the domain is persistent or not */ persistent = virDomainIsPersistent(dom); - vshDebug(ctl, 5, "Domain persistent flag value: %d\n", persistent); + vshDebug(ctl, VSH_ERR_DEBUG, "Domain persistent flag value: %d\n", + persistent); if (persistent < 0) vshPrint(ctl, "%-15s %s\n", _("Persistent:"), _("unknown")); else @@ -3288,7 +3289,7 @@ cmdSetvcpus(vshControl *ctl, const vshCm /* If the --maximum flag was given, we need to ensure only the --config flag is in effect as well */ if (maximum) { - vshDebug(ctl, 5, "--maximum flag was given\n"); + vshDebug(ctl, VSH_ERR_DEBUG, "--maximum flag was given\n"); /* If neither the --config nor --live flags were given, OR if just the --live flag was given, we need to error out @@ -4625,7 +4626,8 @@ repoll: if ( timeout && ((int)(curr.tv_sec - start.tv_sec) * 1000 + \ (int)(curr.tv_usec - start.tv_usec) / 1000) > timeout * 1000 ) { /* suspend the domain when migration timeouts. */ - vshDebug(ctl, 5, "suspend the domain when migration timeouts\n"); + vshDebug(ctl, VSH_ERR_DEBUG, + "suspend the domain when migration timeouts\n"); virDomainSuspend(dom); timeout = 0; } @@ -6859,7 +6861,8 @@ cmdPoolList(vshControl *ctl, const vshCm /* Retrieve the persistence status of the pool */ if (details) { persistent = virStoragePoolIsPersistent(pool); - vshDebug(ctl, 5, "Persistent flag value: %d\n", persistent); + vshDebug(ctl, VSH_ERR_DEBUG, "Persistent flag value: %d\n", + persistent); if (persistent < 0) poolInfoTexts[i].persistent = vshStrdup(ctl, _("unknown")); else @@ -7050,19 +7053,19 @@ cmdPoolList(vshControl *ctl, const vshCm availStrLength = stringLength; /* Display the string lengths for debugging. */ - vshDebug(ctl, 5, "Longest name string = %lu chars\n", + vshDebug(ctl, VSH_ERR_DEBUG, "Longest name string = %lu chars\n", (unsigned long) nameStrLength); - vshDebug(ctl, 5, "Longest state string = %lu chars\n", + vshDebug(ctl, VSH_ERR_DEBUG, "Longest state string = %lu chars\n", (unsigned long) stateStrLength); - vshDebug(ctl, 5, "Longest autostart string = %lu chars\n", + vshDebug(ctl, VSH_ERR_DEBUG, "Longest autostart string = %lu chars\n", (unsigned long) autostartStrLength); - vshDebug(ctl, 5, "Longest persistent string = %lu chars\n", + vshDebug(ctl, VSH_ERR_DEBUG, "Longest persistent string = %lu chars\n", (unsigned long) persistStrLength); - vshDebug(ctl, 5, "Longest capacity string = %lu chars\n", + vshDebug(ctl, VSH_ERR_DEBUG, "Longest capacity string = %lu chars\n", (unsigned long) capStrLength); - vshDebug(ctl, 5, "Longest allocation string = %lu chars\n", + vshDebug(ctl, VSH_ERR_DEBUG, "Longest allocation string = %lu chars\n", (unsigned long) allocStrLength); - vshDebug(ctl, 5, "Longest available string = %lu chars\n", + vshDebug(ctl, VSH_ERR_DEBUG, "Longest available string = %lu chars\n", (unsigned long) availStrLength); /* Create the output template. Each column is sized according to @@ -7334,7 +7337,8 @@ cmdPoolInfo(vshControl *ctl, const vshCm /* Check and display whether the pool is persistent or not */ persistent = virStoragePoolIsPersistent(pool); - vshDebug(ctl, 5, "Pool persistent flag value: %d\n", persistent); + vshDebug(ctl, VSH_ERR_DEBUG, "Pool persistent flag value: %d\n", + persistent); if (persistent < 0) vshPrint(ctl, "%-15s %s\n", _("Persistent:"), _("unknown")); else @@ -7342,7 +7346,8 @@ cmdPoolInfo(vshControl *ctl, const vshCm /* Check and display whether the pool is autostarted or not */ virStoragePoolGetAutostart(pool, &autostart); - vshDebug(ctl, 5, "Pool autostart flag value: %d\n", autostart); + vshDebug(ctl, VSH_ERR_DEBUG, "Pool autostart flag value: %d\n", + autostart); if (autostart < 0) vshPrint(ctl, "%-15s %s\n", _("Autostart:"), _("no autostart")); else @@ -7543,31 +7548,37 @@ cmdVolCreateAs(vshControl *ctl, const vs if (snapshotStrVol) { /* Lookup snapshot backing volume. Try the backing-vol * parameter as a name */ - vshDebug(ctl, 5, "%s: Look up backing store volume '%s' as name\n", + vshDebug(ctl, VSH_ERR_DEBUG, + "%s: Look up backing store volume '%s' as name\n", cmd->def->name, snapshotStrVol); virStorageVolPtr snapVol = virStorageVolLookupByName(pool, snapshotStrVol); if (snapVol) - vshDebug(ctl, 5, "%s: Backing store volume found using '%s' as name\n", + vshDebug(ctl, VSH_ERR_DEBUG, + "%s: Backing store volume found using '%s' as name\n", cmd->def->name, snapshotStrVol); if (snapVol == NULL) { /* Snapshot backing volume not found by name. Try the * backing-vol parameter as a key */ - vshDebug(ctl, 5, "%s: Look up backing store volume '%s' as key\n", + vshDebug(ctl, VSH_ERR_DEBUG, + "%s: Look up backing store volume '%s' as key\n", cmd->def->name, snapshotStrVol); snapVol = virStorageVolLookupByKey(ctl->conn, snapshotStrVol); if (snapVol) - vshDebug(ctl, 5, "%s: Backing store volume found using '%s' as key\n", + vshDebug(ctl, VSH_ERR_DEBUG, + "%s: Backing store volume found using '%s' as key\n", cmd->def->name, snapshotStrVol); } if (snapVol == NULL) { /* Snapshot backing volume not found by key. Try the * backing-vol parameter as a path */ - vshDebug(ctl, 5, "%s: Look up backing store volume '%s' as path\n", + vshDebug(ctl, VSH_ERR_DEBUG, + "%s: Look up backing store volume '%s' as path\n", cmd->def->name, snapshotStrVol); snapVol = virStorageVolLookupByPath(ctl->conn, snapshotStrVol); if (snapVol) - vshDebug(ctl, 5, "%s: Backing store volume found using '%s' as path\n", + vshDebug(ctl, VSH_ERR_DEBUG, + "%s: Backing store volume found using '%s' as path\n", cmd->def->name, snapshotStrVol); } if (snapVol == NULL) { @@ -8504,11 +8515,16 @@ cmdVolList(vshControl *ctl, const vshCmd allocStrLength = stringLength; /* Display the string lengths for debugging */ - vshDebug(ctl, 5, "Longest name string = %zu chars\n", nameStrLength); - vshDebug(ctl, 5, "Longest path string = %zu chars\n", pathStrLength); - vshDebug(ctl, 5, "Longest type string = %zu chars\n", typeStrLength); - vshDebug(ctl, 5, "Longest capacity string = %zu chars\n", capStrLength); - vshDebug(ctl, 5, "Longest allocation string = %zu chars\n", allocStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, + "Longest name string = %zu chars\n", nameStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, + "Longest path string = %zu chars\n", pathStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, + "Longest type string = %zu chars\n", typeStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, + "Longest capacity string = %zu chars\n", capStrLength); + vshDebug(ctl, VSH_ERR_DEBUG, + "Longest allocation string = %zu chars\n", allocStrLength); /* Create the output template */ ret = virAsprintf(&outputStr, @@ -12449,7 +12465,7 @@ vshCommandOptDomainBy(vshControl *ctl, c if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, VSH_ERR_INFO, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -12458,20 +12474,21 @@ vshCommandOptDomainBy(vshControl *ctl, c /* try it by ID */ if (flag & VSH_BYID) { if (virStrToLong_i(n, NULL, 10, &id) == 0 && id >= 0) { - vshDebug(ctl, 5, "%s: <%s> seems like domain ID\n", + vshDebug(ctl, VSH_ERR_DEBUG, + "%s: <%s> seems like domain ID\n", cmd->def->name, optname); dom = virDomainLookupByID(ctl->conn, id); } } /* try it by UUID */ if (dom==NULL && (flag & VSH_BYUUID) && strlen(n)==VIR_UUID_STRING_BUFLEN-1) { - vshDebug(ctl, 5, "%s: <%s> trying as domain UUID\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as domain UUID\n", cmd->def->name, optname); dom = virDomainLookupByUUIDString(ctl->conn, n); } /* try it by NAME */ if (dom==NULL && (flag & VSH_BYNAME)) { - vshDebug(ctl, 5, "%s: <%s> trying as domain NAME\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as domain NAME\n", cmd->def->name, optname); dom = virDomainLookupByName(ctl->conn, n); } @@ -12495,7 +12512,7 @@ vshCommandOptNetworkBy(vshControl *ctl, if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, VSH_ERR_INFO, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -12503,13 +12520,13 @@ vshCommandOptNetworkBy(vshControl *ctl, /* try it by UUID */ if ((flag & VSH_BYUUID) && (strlen(n) == VIR_UUID_STRING_BUFLEN-1)) { - vshDebug(ctl, 5, "%s: <%s> trying as network UUID\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as network UUID\n", cmd->def->name, optname); network = virNetworkLookupByUUIDString(ctl->conn, n); } /* try it by NAME */ if (network==NULL && (flag & VSH_BYNAME)) { - vshDebug(ctl, 5, "%s: <%s> trying as network NAME\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as network NAME\n", cmd->def->name, optname); network = virNetworkLookupByName(ctl->conn, n); } @@ -12534,7 +12551,7 @@ vshCommandOptNWFilterBy(vshControl *ctl, if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, VSH_ERR_INFO, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -12542,13 +12559,13 @@ vshCommandOptNWFilterBy(vshControl *ctl, /* try it by UUID */ if ((flag & VSH_BYUUID) && (strlen(n) == VIR_UUID_STRING_BUFLEN-1)) { - vshDebug(ctl, 5, "%s: <%s> trying as nwfilter UUID\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as nwfilter UUID\n", cmd->def->name, optname); nwfilter = virNWFilterLookupByUUIDString(ctl->conn, n); } /* try it by NAME */ if (nwfilter == NULL && (flag & VSH_BYNAME)) { - vshDebug(ctl, 5, "%s: <%s> trying as nwfilter NAME\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as nwfilter NAME\n", cmd->def->name, optname); nwfilter = virNWFilterLookupByName(ctl->conn, n); } @@ -12572,7 +12589,7 @@ vshCommandOptInterfaceBy(vshControl *ctl if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, VSH_ERR_INFO, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -12580,13 +12597,13 @@ vshCommandOptInterfaceBy(vshControl *ctl /* try it by NAME */ if ((flag & VSH_BYNAME)) { - vshDebug(ctl, 5, "%s: <%s> trying as interface NAME\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as interface NAME\n", cmd->def->name, optname); iface = virInterfaceLookupByName(ctl->conn, n); } /* try it by MAC */ if ((iface == NULL) && (flag & VSH_BYMAC)) { - vshDebug(ctl, 5, "%s: <%s> trying as interface MAC\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as interface MAC\n", cmd->def->name, optname); iface = virInterfaceLookupByMACString(ctl->conn, n); } @@ -12607,7 +12624,7 @@ vshCommandOptPoolBy(vshControl *ctl, con if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, VSH_ERR_INFO, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -12615,13 +12632,13 @@ vshCommandOptPoolBy(vshControl *ctl, con /* try it by UUID */ if ((flag & VSH_BYUUID) && (strlen(n) == VIR_UUID_STRING_BUFLEN-1)) { - vshDebug(ctl, 5, "%s: <%s> trying as pool UUID\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as pool UUID\n", cmd->def->name, optname); pool = virStoragePoolLookupByUUIDString(ctl->conn, n); } /* try it by NAME */ if (pool == NULL && (flag & VSH_BYNAME)) { - vshDebug(ctl, 5, "%s: <%s> trying as pool NAME\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as pool NAME\n", cmd->def->name, optname); pool = virStoragePoolLookupByName(ctl->conn, n); } @@ -12653,7 +12670,7 @@ vshCommandOptVolBy(vshControl *ctl, cons if (p) pool = vshCommandOptPoolBy(ctl, cmd, pooloptname, name, flag); - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -12661,19 +12678,19 @@ vshCommandOptVolBy(vshControl *ctl, cons /* try it by name */ if (pool && (flag & VSH_BYNAME)) { - vshDebug(ctl, 5, "%s: <%s> trying as vol name\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as vol name\n", cmd->def->name, optname); vol = virStorageVolLookupByName(pool, n); } /* try it by key */ if (vol == NULL && (flag & VSH_BYUUID)) { - vshDebug(ctl, 5, "%s: <%s> trying as vol key\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as vol key\n", cmd->def->name, optname); vol = virStorageVolLookupByKey(ctl->conn, n); } /* try it by path */ if (vol == NULL && (flag & VSH_BYUUID)) { - vshDebug(ctl, 5, "%s: <%s> trying as vol path\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: <%s> trying as vol path\n", cmd->def->name, optname); vol = virStorageVolLookupByPath(ctl->conn, n); } @@ -12700,7 +12717,8 @@ vshCommandOptSecret(vshControl *ctl, con if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); + vshDebug(ctl, VSH_ERR_DEBUG, + "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name != NULL) *name = n; @@ -12905,7 +12923,7 @@ get_data: last->next = arg; last = arg; - vshDebug(ctl, 4, "%s: %s(%s): %s\n", + vshDebug(ctl, VSH_ERR_INFO, "%s: %s(%s): %s\n", cmd->name, opt->name, opt->type != VSH_OT_BOOL ? _("optdata") : _("bool"), @@ -14019,7 +14037,7 @@ vshParseArgv(vshControl *ctl, int argc, /* parse command */ ctl->imode = false; if (argc - optind == 1) { - vshDebug(ctl, 2, "commands: \"%s\"\n", argv[optind]); + vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[optind]); return vshCommandStringParse(ctl, argv[optind]); } else { return vshCommandArgvParse(ctl, argc - optind, argv + optind);
On Thu, Jun 30, 2011 at 13:52:20 +0530, Supriya Kannery wrote:
Replace magic numbers with loglevel variables.
Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>
--- tools/virsh.c | 112 +++++++++++++++++++++++++++++++++------------------------- 1 file changed, 65 insertions(+), 47 deletions(-)
ACK Jirka
On 07/14/2011 05:51 AM, Jiri Denemark wrote:
On Thu, Jun 30, 2011 at 13:52:20 +0530, Supriya Kannery wrote:
Replace magic numbers with loglevel variables.
Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>
--- tools/virsh.c | 112 +++++++++++++++++++++++++++++++++------------------------- 1 file changed, 65 insertions(+), 47 deletions(-)
ACK
Pushed, after updating AUTHORS and .mailmap to cover the fact that you've now committed under two different emails. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Aligning loglevel values of virsh to that of libvirt. "DEBUG"=0 loglevel, when specified through commandline or env variable, should log all the messages. "ERROR=4" should log only error messages. Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> --- tools/virsh.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) Index: libvirt/tools/virsh.c =================================================================== --- libvirt.orig/tools/virsh.c +++ libvirt/tools/virsh.c @@ -13285,13 +13285,17 @@ vshDebug(vshControl *ctl, int level, con va_list ap; char *str; + /* Aligning log levels to that of libvirt. + * Traces with levels >= user-specified-level + * gets logged into file + */ + if (level < ctl->debug) + return; + va_start(ap, format); - vshOutputLogFile(ctl, VSH_ERR_DEBUG, format, ap); + vshOutputLogFile(ctl, level, format, ap); va_end(ap); - if (level > ctl->debug) - return; - va_start(ap, format); if (virVasprintf(&str, format, ap) < 0) { /* Skip debug messages on low memory */
On Thu, Jun 30, 2011 at 13:52:32 +0530, Supriya Kannery wrote:
Aligning loglevel values of virsh to that of libvirt. "DEBUG"=0 loglevel, when specified through commandline or env variable, should log all the messages. "ERROR=4" should log only error messages.
Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>
--- tools/virsh.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
Index: libvirt/tools/virsh.c =================================================================== --- libvirt.orig/tools/virsh.c +++ libvirt/tools/virsh.c @@ -13285,13 +13285,17 @@ vshDebug(vshControl *ctl, int level, con va_list ap; char *str;
+ /* Aligning log levels to that of libvirt. + * Traces with levels >= user-specified-level + * gets logged into file + */ + if (level < ctl->debug) + return; + va_start(ap, format); - vshOutputLogFile(ctl, VSH_ERR_DEBUG, format, ap); + vshOutputLogFile(ctl, level, format, ap); va_end(ap);
- if (level > ctl->debug) - return; - va_start(ap, format); if (virVasprintf(&str, format, ap) < 0) { /* Skip debug messages on low memory */
This breaks make check for two reasons. First, it reverses the condition but leaves default level unchanged, so instead of not printint anything but errors before the patch it now prints all debug messages by default. Second, you forgot to change -d5 option passed to virsh in tests/virsh-optparse to -d0; the script wants to see all debug messages. In other words, the following patch needs to be squashed in. Jirka diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 092e80d..7b3a25d 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -64,7 +64,7 @@ for args in \ '--count 2 test' \ '--count=2 test' \ ; do - virsh -d5 -c $test_url setvcpus $args >out 2>>err || fail=1 + virsh -d0 -c $test_url setvcpus $args >out 2>>err || fail=1 LC_ALL=C sort out | compare - exp-out || fail=1 done test -s err && fail=1 diff --git a/tools/virsh.c b/tools/virsh.c index dda86cb..6d356d2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -98,6 +98,8 @@ typedef enum { VSH_ERR_ERROR } vshErrorLevel; +#define VSH_DEBUG_DEFAULT VSH_ERR_ERROR + /* * virsh command line grammar: * @@ -13406,15 +13408,17 @@ vshInit(vshControl *ctl) if (ctl->conn) return false; - if (ctl->debug == -1) { + if (ctl->debug == VSH_DEBUG_DEFAULT) { /* log level not set from commandline, check env variable */ debugEnv = getenv("VIRSH_DEBUG"); if (debugEnv) { - if (virStrToLong_i(debugEnv, NULL, 10, &ctl->debug) < 0 || - ctl->debug < VSH_ERR_DEBUG || ctl->debug > VSH_ERR_ERROR) { + int debug; + if (virStrToLong_i(debugEnv, NULL, 10, &debug) < 0 || + debug < VSH_ERR_DEBUG || debug > VSH_ERR_ERROR) { vshError(ctl, "%s", _("VIRSH_DEBUG not set with a valid numeric value")); - ctl->debug = VSH_ERR_DEBUG; + } else { + ctl->debug = debug; } } } @@ -14102,7 +14106,7 @@ main(int argc, char **argv) memset(ctl, 0, sizeof(vshControl)); ctl->imode = true; /* default is interactive mode */ ctl->log_fd = -1; /* Initialize log file descriptor */ - ctl->debug = -1; /* Initialize log level */ + ctl->debug = VSH_DEBUG_DEFAULT; if (!setlocale(LC_ALL, "")) { perror("setlocale");
On 07/14/2011 05:58 AM, Jiri Denemark wrote:
On Thu, Jun 30, 2011 at 13:52:32 +0530, Supriya Kannery wrote:
Aligning loglevel values of virsh to that of libvirt. "DEBUG"=0 loglevel, when specified through commandline or env variable, should log all the messages. "ERROR=4" should log only error messages.
Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>
This breaks make check for two reasons. First, it reverses the condition but leaves default level unchanged, so instead of not printint anything but errors before the patch it now prints all debug messages by default. Second, you forgot to change -d5 option passed to virsh in tests/virsh-optparse to -d0; the script wants to see all debug messages.
In other words, the following patch needs to be squashed in.
Jirka
diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 092e80d..7b3a25d 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -64,7 +64,7 @@ for args in \ '--count 2 test' \ '--count=2 test' \ ; do - virsh -d5 -c $test_url setvcpus $args >out 2>>err || fail=1 + virsh -d0 -c $test_url setvcpus $args >out 2>>err || fail=1 LC_ALL=C sort out | compare - exp-out || fail=1 done test -s err && fail=1 diff --git a/tools/virsh.c b/tools/virsh.c index dda86cb..6d356d2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -98,6 +98,8 @@ typedef enum { VSH_ERR_ERROR } vshErrorLevel;
+#define VSH_DEBUG_DEFAULT VSH_ERR_ERROR + /* * virsh command line grammar: * @@ -13406,15 +13408,17 @@ vshInit(vshControl *ctl) if (ctl->conn) return false;
- if (ctl->debug == -1) { + if (ctl->debug == VSH_DEBUG_DEFAULT) { /* log level not set from commandline, check env variable */ debugEnv = getenv("VIRSH_DEBUG"); if (debugEnv) { - if (virStrToLong_i(debugEnv, NULL, 10, &ctl->debug) < 0 || - ctl->debug < VSH_ERR_DEBUG || ctl->debug > VSH_ERR_ERROR) { + int debug; + if (virStrToLong_i(debugEnv, NULL, 10, &debug) < 0 || + debug < VSH_ERR_DEBUG || debug > VSH_ERR_ERROR) { vshError(ctl, "%s", _("VIRSH_DEBUG not set with a valid numeric value")); - ctl->debug = VSH_ERR_DEBUG; + } else { + ctl->debug = debug; } } } @@ -14102,7 +14106,7 @@ main(int argc, char **argv) memset(ctl, 0, sizeof(vshControl)); ctl->imode = true; /* default is interactive mode */ ctl->log_fd = -1; /* Initialize log file descriptor */ - ctl->debug = -1; /* Initialize log level */ + ctl->debug = VSH_DEBUG_DEFAULT;
if (!setlocale(LC_ALL, "")) { perror("setlocale");
ACK to that delta, so I've pushed the combined patch. Aargh! Correction, I pushed the broken patch first, accidentally, so now I'm pushing the fix separately. :( (I hate the lag between my computer and libvirt.org - I keep forgetting that a 'git push' started in one terminal, followed by 'git rebase --continue' in another, can affect the operation in the first terminal - I really wish 'git push' would remember the HEAD that was in effect at the start of the operation, rather than what is in effect at the time the remote connection is actually established). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/14/2011 05:28 PM, Jiri Denemark wrote:
On Thu, Jun 30, 2011 at 13:52:32 +0530, Supriya Kannery wrote:
va_start(ap, format); if (virVasprintf(&str, format, ap)< 0) { /* Skip debug messages on low memory */
This breaks make check for two reasons. First, it reverses the condition but leaves default level unchanged, so instead of not printint anything but errors before the patch it now prints all debug messages by default. Second, you forgot to change -d5 option passed to virsh in tests/virsh-optparse to -d0; the script wants to see all debug messages.
Jiri, Thanks! for reviewing this patchset. I missed to look at make check. Will take this as a learning while preparing future patches.
In other words, the following patch needs to be squashed in.
Jirka
diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 092e80d..7b3a25d 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -64,7 +64,7 @@ for args in \ '--count 2 test' \ '--count=2 test' \ ; do - virsh -d5 -c $test_url setvcpus $args>out 2>>err || fail=1 + virsh -d0 -c $test_url setvcpus $args>out 2>>err || fail=1 LC_ALL=C sort out | compare - exp-out || fail=1 done test -s err&& fail=1 diff --git a/tools/virsh.c b/tools/virsh.c index dda86cb..6d356d2 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -98,6 +98,8 @@ typedef enum { VSH_ERR_ERROR } vshErrorLevel;
+#define VSH_DEBUG_DEFAULT VSH_ERR_ERROR + /* * virsh command line grammar: * @@ -13406,15 +13408,17 @@ vshInit(vshControl *ctl) if (ctl->conn) return false;
- if (ctl->debug == -1) { + if (ctl->debug == VSH_DEBUG_DEFAULT) {
ctl->debug was initialized to -1, so that, if it is already set from commandline, there is no need to read ENV. My intention was to keep the order as cmdline > ENV and avoid overwriting of cmd loglevel by ENV. If we initialize ctl->debug to VSH_DEBUG_DEFAULT, incase from commandline, user set -d as VSH_ERR_ERROR (or VSH_DEBUG_DEFAULT), then unnecessarily ctl->debug will be overwritten by ENV.
@@ -14102,7 +14106,7 @@ main(int argc, char **argv) memset(ctl, 0, sizeof(vshControl)); ctl->imode = true; /* default is interactive mode */ ctl->log_fd = -1; /* Initialize log file descriptor */ - ctl->debug = -1; /* Initialize log level */ + ctl->debug = VSH_DEBUG_DEFAULT;
if (!setlocale(LC_ALL, "")) { perror("setlocale");
Valid loglevel range for virsh is 0-4. Update virsh man page accordingly. Also explain virsh ENV variables and values. Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> --- tools/virsh.pod | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) Index: libvirt/tools/virsh.pod =================================================================== --- libvirt.orig/tools/virsh.pod +++ libvirt/tools/virsh.pod @@ -72,7 +72,7 @@ instead of the default connection. =item B<-d>, B<--debug> I<LEVEL> Enable debug messages at integer I<LEVEL> and above. I<LEVEL> can -range from 0 (default) to 5. +range from 0 (default) to 4. =item B<-l>, B<--log> I<FILE> @@ -1534,6 +1534,34 @@ of C<virsh> =over 4 +=item VIRSH_DEBUG=<0 to 4> + +Turn on verbose debugging of virsh commands. Valid levels are + +=item * VIRSH_DEBUG=0 + +DEBUG - Messages at ALL levels get logged + +=item * VIRSH_DEBUG=1 + +INFO - Logs messages at levels INFO, NOTICE, WARNING and ERROR + +=item * VIRSH_DEBUG=2 + +NOTICE - Logs messages at levels NOTICE, WARNING and ERROR + +=item * VIRSH_DEBUG=3 + +WARNING - Logs messages at levels WARNING and ERROR + +=item * VIRSH_DEBUG=4 + +ERROR - Messages at only ERROR level gets logged. + +=item VIRSH_LOG_FILE=C<LOGFILE> + +The file to log virsh debug messages. + =item VIRSH_DEFAULT_CONNECT_URI The hypervisor to connect to by default. Set this to a URI, in the same
On Thu, Jun 30, 2011 at 13:52:45 +0530, Supriya Kannery wrote:
Valid loglevel range for virsh is 0-4. Update virsh man page accordingly. Also explain virsh ENV variables and values.
Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>
--- tools/virsh.pod | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
ACK Jirka
On 07/14/2011 05:52 AM, Jiri Denemark wrote:
On Thu, Jun 30, 2011 at 13:52:45 +0530, Supriya Kannery wrote:
Valid loglevel range for virsh is 0-4. Update virsh man page accordingly. Also explain virsh ENV variables and values.
Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>
--- tools/virsh.pod | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
ACK
Pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/30/2011 01:52 PM, Supriya Kannery wrote:
Virsh logging has some basic issues: 1. In code, magic numbers are used for logging rather than loglevel variables. 2. Magic number "5" is used for logging which doesn't map to any loglevel variable. Valid loglevel range is 0-4 3. Usage of loglevel variables doesn't align with that of libvirt logging. In libvirt "DEBUG" loglevel is the superset and logs messages at all other levels. Whereas in virsh, "ERROR" loglevel behaves this way, which needs correction 4. virsh man page and code are inconsistent with respect to loglevels
Following patchset is to address the above mentioned issues.
1/3 - Avoid using magic numbers for logging 2/3 - Align log level usage to that of libvirt 3/3 - Update virsh manpage with related changes
tools/virsh.pod | 30 ++++++++++++ tools/virsh.c | 124 +++++++++++++++++++++++++++--------------------- 2 files changed, 102 insertions(+), 52 deletions(-)
Please review... Thanks, Supriya
On Thu, Jul 14, 2011 at 12:57:41 +0530, Supriya Kannery wrote:
On 06/30/2011 01:52 PM, Supriya Kannery wrote:
Virsh logging has some basic issues: 1. In code, magic numbers are used for logging rather than loglevel variables. 2. Magic number "5" is used for logging which doesn't map to any loglevel variable. Valid loglevel range is 0-4 3. Usage of loglevel variables doesn't align with that of libvirt logging. In libvirt "DEBUG" loglevel is the superset and logs messages at all other levels. Whereas in virsh, "ERROR" loglevel behaves this way, which needs correction 4. virsh man page and code are inconsistent with respect to loglevels
Following patchset is to address the above mentioned issues.
1/3 - Avoid using magic numbers for logging 2/3 - Align log level usage to that of libvirt 3/3 - Update virsh manpage with related changes
tools/virsh.pod | 30 ++++++++++++ tools/virsh.c | 124 +++++++++++++++++++++++++++--------------------- 2 files changed, 102 insertions(+), 52 deletions(-)
Please review...
Yeah, I started reviewing these yesterday and bumped into make check failures which I didn't have time to fully investigate yet. Except for some minor things, the series looks good so far. Jirka
participants (4)
-
Eric Blake -
Jiri Denemark -
Supriya Kannery -
Supriya Kannery