[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]