Re: [libvirt] [V2 Patch] virsh: Change log level order

On Fri, May 20, 2011 at 03:05:28PM +0530, Supriya Kannery wrote:

Change log level order so that messages at all other levels get logged for "DEBUG" level. Replace magic numbers with corresponding VSH_ERR_xx.

Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>

--- tools/virsh.c | 119 ++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 67 insertions(+), 52 deletions(-)

Index: libvirt/tools/virsh.c =================================================================== --- libvirt.orig/tools/virsh.c +++ libvirt/tools/virsh.c @@ -91,11 +91,11 @@ static char *progname; * Indicates the level of a log message */ typedef enum { - VSH_ERR_DEBUG = 0, - VSH_ERR_INFO, - VSH_ERR_NOTICE, + VSH_ERR_ERROR = 0, VSH_ERR_WARNING, - VSH_ERR_ERROR + VSH_ERR_NOTICE, + VSH_ERR_INFO, + VSH_ERR_DEBUG } vshErrorLevel;

NACK I don't think this bit is a good idea. In fact I think it should be changed to match the log levels in src/util/logging.h exactly, so we have the same behaviour for virsh and the rest of libvirt.

@@ -2146,7 +2146,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 @@ -2928,7 +2929,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 @@ -4020,7 +4021,7 @@ 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; } @@ -6125,7 +6126,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 @@ -6316,19 +6318,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 @@ -6600,7 +6602,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 @@ -6608,7 +6611,7 @@ 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 @@ -6806,31 +6809,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) { @@ -7777,11 +7786,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, @@ -11535,7 +11549,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_DEBUG, "%s: found option <%s>: %s\n", cmd->def->name, optname, n);

if (name) @@ -11544,20 +11558,20 @@ 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); } @@ -11581,7 +11595,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_DEBUG, "%s: found option <%s>: %s\n", cmd->def->name, optname, n);

if (name) @@ -11589,13 +11603,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); } @@ -11620,7 +11634,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_DEBUG, "%s: found option <%s>: %s\n", cmd->def->name, optname, n);

if (name) @@ -11628,13 +11642,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); } @@ -11658,7 +11672,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_DEBUG, "%s: found option <%s>: %s\n", cmd->def->name, optname, n);

if (name) @@ -11666,13 +11680,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); } @@ -11693,7 +11707,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_DEBUG, "%s: found option <%s>: %s\n", cmd->def->name, optname, n);

if (name) @@ -11701,13 +11715,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); } @@ -11739,7 +11753,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) @@ -11747,19 +11761,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); } @@ -11786,7 +11800,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; @@ -11991,7 +12006,7 @@ get_data: last->next = arg; last = arg;

- vshDebug(ctl, 4, "%s: %s(%s): %s\n", + vshDebug(ctl, VSH_ERR_DEBUG, "%s: %s(%s): %s\n", cmd->name, opt->name, opt->type != VSH_OT_BOOL ? _("optdata") : _("bool"), @@ -12416,7 +12431,7 @@ vshInit(vshControl *ctl) 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) { + ctl->debug > VSH_ERR_DEBUG || ctl->debug < VSH_ERR_ERROR) { vshError(ctl, "%s", _("VIRSH_DEBUG not set with a valid numeric value")); ctl->debug = VSH_ERR_DEBUG; @@ -13088,7 +13103,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_NOTICE, "commands: \"%s\"\n", argv[optind]); return vshCommandStringParse(ctl, argv[optind]); } else { return vshCommandArgvParse(ctl, argc - optind, argv + optind);

ACK to all these changes though, hardcoding the numbers instead of using the enums was clearly bad. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|