[libvirt] [RFC Patch 0/3]virsh: Enable env support for virsh logging
Defining environment variables for debug log-level and log-file will avoid specifying the same in each virsh command. Following is a patchset for enabling env variable support for virsh logging. Two new environment variables are defined: a. VIRSH_DEBUG=<log_level> log_level can be a value between 0 - 4, where 0 -> "ERROR" 1 -> "WARNING" 2 -> "NOTICE" 3 -> "INFO" 4 -> "DEBUG" b. VIRSH_LOG_FILE=<logfile-path-and-name> logfile-path-and-name is the name and complete path for the log file. The above two variables are independent. if log_level is specified without setting a log file name, then, logs will be displayed in stdio. If logfile-path-and-name specified without setting log_level, then, default log_level (DEBUG) will be used to log to the specified file. virsh commandline parameters (-l and -d) get precedence over environment variables and will override respective env variable values. 1/3 - Use env variables to control virsh logging 2/3 - Change log level order to make "DEBUG" the superset 3/3 - Related changes to virsh manpage
Use variables VIRSH_DEBUG and VIRSH_LOG_FILE for controlling virsh logging. Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> --- tools/virsh.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) Index: libvirt/tools/virsh.c =================================================================== --- libvirt.orig/tools/virsh.c +++ libvirt/tools/virsh.c @@ -12150,9 +12150,31 @@ vshError(vshControl *ctl, const char *fo static bool vshInit(vshControl *ctl) { + char *debugEnv; + if (ctl->conn) return false; + if (ctl->debug == -1) { + /* 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)) { + vshError(ctl, "%s", _("VIRSH_DEBUG not set with a valid numeric value")); + ctl->debug = VSH_ERR_DEBUG; + } + } + } + + if (ctl->logfile == NULL) { + /* log file not set from cmdline */ + debugEnv = getenv("VIRSH_LOG_FILE"); + if (debugEnv && *debugEnv){ + ctl->logfile = vshStrdup(ctl, debugEnv); + } + } + vshOpenLogFile(ctl); /* set up the library error handler */ @@ -12248,14 +12270,15 @@ vshOutputLogFile(vshControl *ctl, int lo */ gettimeofday(&stTimeval, NULL); stTm = localtime(&stTimeval.tv_sec); - virBufferAsprintf(&buf, "[%d.%02d.%02d %02d:%02d:%02d %s] ", + virBufferAsprintf(&buf, "[%d.%02d.%02d %02d:%02d:%02d %s %d] ", (1900 + stTm->tm_year), (1 + stTm->tm_mon), stTm->tm_mday, stTm->tm_hour, stTm->tm_min, stTm->tm_sec, - SIGN_NAME); + SIGN_NAME, + getpid()); switch (log_level) { case VSH_ERR_DEBUG: lvl = LVL_DEBUG; @@ -12574,7 +12597,7 @@ vshUsage(void) " options:\n" " -c | --connect <uri> hypervisor connection URI\n" " -r | --readonly connect readonly\n" - " -d | --debug <num> debug level [0-5]\n" + " -d | --debug <num> debug level [0-4]\n" " -h | --help this help\n" " -q | --quiet quiet mode\n" " -t | --timing print timing information\n" @@ -12840,6 +12863,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; /* Initiatize log level */ if ((defaultConn = getenv("VIRSH_DEFAULT_CONNECT_URI"))) { ctl->name = vshStrdup(ctl, defaultConn);
On 05/09/2011 01:08 AM, Supriya Kannery wrote:
Use variables VIRSH_DEBUG and VIRSH_LOG_FILE for controlling virsh logging.
Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> --- tools/virsh.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-)
+ if (debugEnv) { + if ((virStrToLong_i(debugEnv, NULL, 10, &ctl->debug) < 0) || + (ctl->debug < VSH_ERR_DEBUG) || (ctl->debug > VSH_ERR_ERROR)) {
Indentation was inconsistent here, and resulted in a long line. I reformatted a bit.
@@ -12248,14 +12270,15 @@ vshOutputLogFile(vshControl *ctl, int lo */ gettimeofday(&stTimeval, NULL); stTm = localtime(&stTimeval.tv_sec); - virBufferAsprintf(&buf, "[%d.%02d.%02d %02d:%02d:%02d %s] ", + virBufferAsprintf(&buf, "[%d.%02d.%02d %02d:%02d:%02d %s %d] ", (1900 + stTm->tm_year), (1 + stTm->tm_mon), stTm->tm_mday, stTm->tm_hour, stTm->tm_min, stTm->tm_sec, - SIGN_NAME); + SIGN_NAME, + getpid());
pid_t is not necessarily int, so this needs a cast.
switch (log_level) { case VSH_ERR_DEBUG: lvl = LVL_DEBUG; @@ -12574,7 +12597,7 @@ vshUsage(void) " options:\n" " -c | --connect <uri> hypervisor connection URI\n" " -r | --readonly connect readonly\n" - " -d | --debug <num> debug level [0-5]\n" + " -d | --debug <num> debug level [0-4]\n"
Interesting - and you're right that level 5 was never used for anything; level 4 gets the same behavior.
" -h | --help this help\n" " -q | --quiet quiet mode\n" " -t | --timing print timing information\n" @@ -12840,6 +12863,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; /* Initiatize log level */
s/Initiatize/Initialize/ ACK with those changes, so I pushed this one. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Change log level order so that messages at all other levels get logged for "DEBUG" level. Replace log level "5" with log level "4" (DEBUG). Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> --- tools/virsh.c | 100 +++++++++++++++++++++++++++++----------------------------- 1 file changed, 50 insertions(+), 50 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; /* @@ -2103,7 +2103,7 @@ 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, 4, "Domain persistent flag value: %d\n", persistent); if (persistent < 0) vshPrint(ctl, "%-15s %s\n", _("Persistent:"), _("unknown")); else @@ -2885,7 +2885,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, 4, "--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 @@ -3950,7 +3950,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, 4, "suspend the domain when migration timeouts\n"); virDomainSuspend(dom); timeout = 0; } @@ -6055,7 +6055,7 @@ 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, 4, "Persistent flag value: %d\n", persistent); if (persistent < 0) poolInfoTexts[i].persistent = vshStrdup(ctl, _("unknown")); else @@ -6246,19 +6246,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, 4, "Longest name string = %lu chars\n", (unsigned long) nameStrLength); - vshDebug(ctl, 5, "Longest state string = %lu chars\n", + vshDebug(ctl, 4, "Longest state string = %lu chars\n", (unsigned long) stateStrLength); - vshDebug(ctl, 5, "Longest autostart string = %lu chars\n", + vshDebug(ctl, 4, "Longest autostart string = %lu chars\n", (unsigned long) autostartStrLength); - vshDebug(ctl, 5, "Longest persistent string = %lu chars\n", + vshDebug(ctl, 4, "Longest persistent string = %lu chars\n", (unsigned long) persistStrLength); - vshDebug(ctl, 5, "Longest capacity string = %lu chars\n", + vshDebug(ctl, 4, "Longest capacity string = %lu chars\n", (unsigned long) capStrLength); - vshDebug(ctl, 5, "Longest allocation string = %lu chars\n", + vshDebug(ctl, 4, "Longest allocation string = %lu chars\n", (unsigned long) allocStrLength); - vshDebug(ctl, 5, "Longest available string = %lu chars\n", + vshDebug(ctl, 4, "Longest available string = %lu chars\n", (unsigned long) availStrLength); /* Create the output template. Each column is sized according to @@ -6530,7 +6530,7 @@ 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, 4, "Pool persistent flag value: %d\n", persistent); if (persistent < 0) vshPrint(ctl, "%-15s %s\n", _("Persistent:"), _("unknown")); else @@ -6538,7 +6538,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, 4, "Pool autostart flag value: %d\n", autostart); if (autostart < 0) vshPrint(ctl, "%-15s %s\n", _("Autostart:"), _("no autostart")); else @@ -6736,31 +6736,31 @@ 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, 4, "%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, 4, "%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, 4, "%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, 4, "%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, 4, "%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, 4, "%s: Backing store volume found using '%s' as path\n", cmd->def->name, snapshotStrVol); } if (snapVol == NULL) { @@ -7707,11 +7707,11 @@ 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, 4, "Longest name string = %zu chars\n", nameStrLength); + vshDebug(ctl, 4, "Longest path string = %zu chars\n", pathStrLength); + vshDebug(ctl, 4, "Longest type string = %zu chars\n", typeStrLength); + vshDebug(ctl, 4, "Longest capacity string = %zu chars\n", capStrLength); + vshDebug(ctl, 4, "Longest allocation string = %zu chars\n", allocStrLength); /* Create the output template */ ret = virAsprintf(&outputStr, @@ -11410,7 +11410,7 @@ vshCommandOptDomainBy(vshControl *ctl, c if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, 4, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -11419,20 +11419,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, 4, "%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, 4, "%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, 4, "%s: <%s> trying as domain NAME\n", cmd->def->name, optname); dom = virDomainLookupByName(ctl->conn, n); } @@ -11456,7 +11456,7 @@ vshCommandOptNetworkBy(vshControl *ctl, if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, 4, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -11464,13 +11464,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, 4, "%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, 4, "%s: <%s> trying as network NAME\n", cmd->def->name, optname); network = virNetworkLookupByName(ctl->conn, n); } @@ -11495,7 +11495,7 @@ vshCommandOptNWFilterBy(vshControl *ctl, if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, 4, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -11503,13 +11503,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, 4, "%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, 4, "%s: <%s> trying as nwfilter NAME\n", cmd->def->name, optname); nwfilter = virNWFilterLookupByName(ctl->conn, n); } @@ -11533,7 +11533,7 @@ vshCommandOptInterfaceBy(vshControl *ctl if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, 4, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -11541,13 +11541,13 @@ vshCommandOptInterfaceBy(vshControl *ctl /* try it by NAME */ if ((flag & VSH_BYNAME)) { - vshDebug(ctl, 5, "%s: <%s> trying as interface NAME\n", + vshDebug(ctl, 4, "%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, 4, "%s: <%s> trying as interface MAC\n", cmd->def->name, optname); iface = virInterfaceLookupByMACString(ctl->conn, n); } @@ -11568,7 +11568,7 @@ vshCommandOptPoolBy(vshControl *ctl, con if (vshCommandOptString(cmd, optname, &n) <= 0) return NULL; - vshDebug(ctl, 5, "%s: found option <%s>: %s\n", + vshDebug(ctl, 4, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -11576,13 +11576,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, 4, "%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, 4, "%s: <%s> trying as pool NAME\n", cmd->def->name, optname); pool = virStoragePoolLookupByName(ctl->conn, n); } @@ -11614,7 +11614,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, 4, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name) @@ -11622,19 +11622,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, 4, "%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, 4, "%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, 4, "%s: <%s> trying as vol path\n", cmd->def->name, optname); vol = virStorageVolLookupByPath(ctl->conn, n); } @@ -11661,7 +11661,7 @@ 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, 4, "%s: found option <%s>: %s\n", cmd->def->name, optname, n); if (name != NULL) *name = n; @@ -12160,7 +12160,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_ERROR) || (ctl->debug > VSH_ERR_DEBUG)) { vshError(ctl, "%s", _("VIRSH_DEBUG not set with a valid numeric value")); ctl->debug = VSH_ERR_DEBUG; }
On 05/09/2011 01:08 AM, Supriya Kannery wrote:
Change log level order so that messages at all other levels get logged for "DEBUG" level. Replace log level "5" with log level "4" (DEBUG).
Signed-off-by: Supriya Kannery <supriyak@in.ibm.com> --- tools/virsh.c | 100 +++++++++++++++++++++++++++++----------------------------- 1 file changed, 50 insertions(+), 50 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;
I like this part.
/* @@ -2103,7 +2103,7 @@ 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, 4, "Domain persistent flag value: %d\n", persistent);
Please resubmit by using VSH_ERR_DEBUG rather than the magic number '5' (or '4') in all of these vshDebug statements. If we're going to clean up this code, let's clean it up right. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/20/2011 05:13 AM, Eric Blake wrote:
On 05/09/2011 01:08 AM, Supriya Kannery wrote:
@@ -2103,7 +2103,7 @@ 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, 4, "Domain persistent flag value: %d\n", persistent);
Please resubmit by using VSH_ERR_DEBUG rather than the magic number '5' (or '4') in all of these vshDebug statements. If we're going to clean up this code, let's clean it up right.
Thanks! for reviewing this patchset. Posted version 2 of this patch, which replaces magic numbers with corresponding VSH_ERR_xx. http://www.mail-archive.com/libvir-list@redhat.com/msg38280.html Rgds, Supriya
Update virsh man page with usage of new env variables 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 to 4 (default). =item B<-l>, B<--log> I<FILE> @@ -1340,6 +1340,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 + +Messages at level ERROR + +=item * VIRSH_DEBUG=1 + +Messages at levels WARNING and ERROR + +=item * VIRSH_DEBUG=2 + +Messages at levels NOTICE, WARNING and ERROR + +=item * VIRSH_DEBUG=3 + +Messages at levels INFO, NOTICE, WARNING and ERROR + +=item * VIRSH_DEBUG=4 + +Messages at level DEBUG and all other lower levels. + +=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 05/09/2011 01:07 AM, Supriya Kannery wrote:
Defining environment variables for debug log-level and log-file will avoid specifying the same in each virsh command. Following is a patchset for enabling env variable support for virsh logging.
Two new environment variables are defined:
a. VIRSH_DEBUG=<log_level> log_level can be a value between 0 - 4, where 0 -> "ERROR" 1 -> "WARNING" 2 -> "NOTICE" 3 -> "INFO" 4 -> "DEBUG"
b. VIRSH_LOG_FILE=<logfile-path-and-name> logfile-path-and-name is the name and complete path for the log file.
The above two variables are independent. if log_level is specified without setting a log file name, then, logs will be displayed in stdio. If logfile-path-and-name specified without setting log_level, then, default log_level (DEBUG) will be used to log to the specified file.
Seems reasonable to me. I'm now in the middle of reviewing the patch set. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, May 19, 2011 at 12:19:11PM -0600, Eric Blake wrote:
On 05/09/2011 01:07 AM, Supriya Kannery wrote:
Defining environment variables for debug log-level and log-file will avoid specifying the same in each virsh command. Following is a patchset for enabling env variable support for virsh logging.
Two new environment variables are defined:
a. VIRSH_DEBUG=<log_level> log_level can be a value between 0 - 4, where 0 -> "ERROR" 1 -> "WARNING" 2 -> "NOTICE" 3 -> "INFO" 4 -> "DEBUG"
This is exactly the opposite to LIBVIRT_DEBUG=<log level> which has * 1: DEBUG * 2: INFO * 3: WARNING * 4: ERROR IMHO, these need to be kept the same.
b. VIRSH_LOG_FILE=<logfile-path-and-name> logfile-path-and-name is the name and complete path for the log file.
The above two variables are independent. if log_level is specified without setting a log file name, then, logs will be displayed in stdio. If logfile-path-and-name specified without setting log_level, then, default log_level (DEBUG) will be used to log to the specified file.
Seems reasonable to me. I'm now in the middle of reviewing the patch set.
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 :|
On 05/20/2011 03:19 PM, Daniel P. Berrange wrote:
On Thu, May 19, 2011 at 12:19:11PM -0600, Eric Blake wrote:
On 05/09/2011 01:07 AM, Supriya Kannery wrote:
a. VIRSH_DEBUG=<log_level> log_level can be a value between 0 - 4, where 0 -> "ERROR" 1 -> "WARNING" 2 -> "NOTICE" 3 -> "INFO" 4 -> "DEBUG"
This is exactly the opposite to LIBVIRT_DEBUG=<log level> which has
* 1: DEBUG * 2: INFO * 3: WARNING * 4: ERROR
IMHO, these need to be kept the same.
Through out virsh.c, the vshDebug is called with the assumption that DEBUG=4 or 5. Also, any higher log level record than user specified log level, doesn't get displayed as per vshDebug() code. So there was no match between the enum definition and the code written. vshDebug(vshControl *ctl, int level, const char *format, ...) { ... vshOutputLogFile(ctl, 5, format, ap); va_end(ap); if (level > ctl->debug) return; va_start(ap, format); ... } When user choose "DEBUG", the expectation is that it is the superset of all log records. Usually, higher the log level value, more records are expected. I can try changing libvirt as well to align to this norm, if there are no major compatibility issues. Please comment.
On Fri, May 20, 2011 at 04:17:12PM +0530, Supriya Kannery wrote:
On 05/20/2011 03:19 PM, Daniel P. Berrange wrote:
On Thu, May 19, 2011 at 12:19:11PM -0600, Eric Blake wrote:
On 05/09/2011 01:07 AM, Supriya Kannery wrote:
a. VIRSH_DEBUG=<log_level> log_level can be a value between 0 - 4, where 0 -> "ERROR" 1 -> "WARNING" 2 -> "NOTICE" 3 -> "INFO" 4 -> "DEBUG"
This is exactly the opposite to LIBVIRT_DEBUG=<log level> which has
* 1: DEBUG * 2: INFO * 3: WARNING * 4: ERROR
IMHO, these need to be kept the same.
Through out virsh.c, the vshDebug is called with the assumption that DEBUG=4 or 5. Also, any higher log level record than user specified log level, doesn't get displayed as per vshDebug() code. So there was no match between the enum definition and the code written. vshDebug(vshControl *ctl, int level, const char *format, ...) { ... vshOutputLogFile(ctl, 5, format, ap); va_end(ap);
if (level > ctl->debug) return;
va_start(ap, format); ... }
When user choose "DEBUG", the expectation is that it is the superset of all log records. Usually, higher the log level value, more records are expected. I can try changing libvirt as well to align to this norm, if there are no major compatibility issues. Please comment.
We can't really change libvirt log level, IMHO, because it is too widely used and thus changing it would cause endless confusion. Likewise I think that if virsh used different log level numbers from libvirt, this will cause confusion, so I think the only option is to make the virsh log levels work in exactly the same way as existing libvirt code 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 :|
On 05/20/2011 04:46 AM, Daniel P. Berrange wrote:
This is exactly the opposite to LIBVIRT_DEBUG=<log level> which has
* 1: DEBUG * 2: INFO * 3: WARNING * 4: ERROR
IMHO, these need to be kept the same.
Through out virsh.c, the vshDebug is called with the assumption that DEBUG=4 or 5. Also, any higher log level record than user specified log level, doesn't get displayed as per vshDebug() code. So there was no match between the enum definition and the code written. vshDebug(vshControl *ctl, int level, const char *format, ...) { ... vshOutputLogFile(ctl, 5, format, ap); va_end(ap);
if (level > ctl->debug) return;
Based on Daniel's comments, that means that we need to change this logic; instead of being 'if (level > ctl->debug)' where the most verbose levels are high, we should instead use 'if (level <= ctl->debug)'. Or, to make it even easier, we should have a helper function: if (vshSkipLog(ctl, level)) return; in one patch, then the next patch has to touch _only_ vshSkipLog to implement changed logic in compliance with the rest of libvirtd logging levels. Ultimately, I still think this is all doable without breaking 'virsh -d' handling, but it may take some careful effort, and if it slips until after 0.9.2, that's okay with me.
We can't really change libvirt log level, IMHO, because it is too widely used and thus changing it would cause endless confusion. Likewise I think that if virsh used different log level numbers from libvirt, this will cause confusion, so I think the only option is to make the virsh log levels work in exactly the same way as existing libvirt code
Daniel
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Supriya Kannery -
Supriya Kannery