3:22 a.m.
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(-)
3:22 a.m.
New subject: [libvirt] [Patch 1/3]virsh: Avoid using magic numbers for logging
Replace magic numbers with loglevel variables.
Signed-off-by: Supriya Kannery <supriyak(a)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);
6:51 a.m.
New subject: [libvirt] [Patch 1/3]virsh: Avoid using magic numbers for logging
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(a)in.ibm.com>
---
tools/virsh.c | 112 +++++++++++++++++++++++++++++++++-------------------------
1 file changed, 65 insertions(+), 47 deletions(-)
ACK
Jirka
8:05 a.m.
New subject: [libvirt] [Patch 1/3]virsh: Avoid using magic numbers for logging
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:22 a.m.
New subject: [libvirt] [Patch 2/3]virsh: Make "DEBUG" loglevel the superset
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(a)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 */
6:58 a.m.
New subject: [libvirt] [Patch 2/3]virsh: Make "DEBUG" loglevel the superset
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(a)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");
8:13 a.m.
New subject: [libvirt] [Patch 2/3]virsh: Make "DEBUG" loglevel the superset
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:25 a.m.
New subject: [libvirt] [Patch 2/3]virsh: Make "DEBUG" loglevel the superset
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");
3:22 a.m.
New subject: [libvirt] [Patch 3/3]virsh: Update virsh man page
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(a)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
6:52 a.m.
New subject: [libvirt] [Patch 3/3]virsh: Update virsh man page
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(a)in.ibm.com>
---
tools/virsh.pod | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
ACK
Jirka
8:26 a.m.
New subject: [libvirt] [Patch 3/3]virsh: Update virsh man page
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(a)in.ibm.com>
>
> ---
> tools/virsh.pod | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
ACK
Pushed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:27 a.m.
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
4:32 a.m.
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
4879
days inactive
4894
days old
12 comments
4 participants
participants (4)
-
Eric Blake -
Jiri Denemark -
Supriya Kannery -
Supriya Kannery