[ovs-dev] [PATCH] ovs-bugtool: Added --ovs option to get only ovs related information

Reid Price reid at nicira.com
Wed Jul 11 01:57:12 UTC 2012


A few comments inline

On Tue, Jul 10, 2012 at 12:07 PM, Arun Sharma <arun.sharma at calsoftinc.com>wrote:
>
> diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/
> ovs-bugtool.in
> index 3bafa13..af2bb6e 100755
> --- a/utilities/bugtool/ovs-bugtool.in
> +++ b/utilities/bugtool/ovs-bugtool.in
> @@ -404,6 +404,10 @@ def main(argv = None):
>      global ANSWER_YES_TO_ALL, SILENT_MODE
>      global entries, data, dbg, unlimited_data
>
> +    # Filter flags
> +    only_ovs_info = False
> +    filter_data = False
> +
>      # we need access to privileged files, exit if we are not running as
> root
>      if os.getuid() != 0:
>          print >>sys.stderr, "Error: ovs-bugtool must be run as root"
>
> @@ -493,6 +500,9 @@ def main(argv = None):
>      if ANSWER_YES_TO_ALL:
>          output("Warning: '--yestoall' argument provided, will not prompt
> for individual files.")
>
> +    if only_ovs_info:
> +        filter_data = True
>

It seems like filter_data is redundantly equivalent to only_ovs_info.  Is
this desired?


@@ -561,7 +571,7 @@ exclude those logs from the archive.
>      cmd_output(CAP_MULTIPATH, [DMSETUP, 'table'])
>      func_output(CAP_MULTIPATH, 'multipathd_topology', multipathd_topology)
>      cmd_output(CAP_MULTIPATH, [MPPUTIL, '-a'])
> -    if CAP_MULTIPATH in entries:
> +    if CAP_MULTIPATH in entries and not filter_data:
>          dump_rdac_groups(CAP_MULTIPATH)
>
>      tree_output(CAP_NETWORK_CONFIG, SYSCONFIG_NETWORK_SCRIPTS, IFCFG_RE)
> @@ -609,10 +619,12 @@ exclude those logs from the archive.
>              vspidfile = open(OPENVSWITCH_VSWITCHD_PID)
>              vspid = int(vspidfile.readline().strip())
>              vspidfile.close()
> -            for b in bond_list(vspid):
> -                cmd_output(CAP_NETWORK_STATUS,
> -                           [OVS_APPCTL, '-t', '@RUNDIR@/ovs-vswitchd.%s.ctl'
> % vspid, '-e' 'bond/show %s' % b],
> -                           'ovs-appctl-bond-show-%s.out' % b)
> +            if not filter_data or only_ovs_info:
>

Won't this always be true, since filter_data == only_ovs_info?
Perhaps you mean 'not filter_data' as you did near line 576 above, or 'not
(filter_data or only_ovs_info)'?


> +                for b in bond_list(vspid):
> +                    cmd_output(CAP_NETWORK_STATUS,
> +                               [OVS_APPCTL, '-t',
> +                                '@RUNDIR@/ovs-vswitchd.%s.ctl' % vspid,
> '-e' 'bond/show %s' % b],
> +                               'ovs-appctl-bond-show-%s.out' % b)
>



> @@ -650,15 +662,30 @@ exclude those logs from the archive.
>      tree_output(CAP_YUM, APT_SOURCES_LIST_D)
>      cmd_output(CAP_YUM, [DPKG_QUERY, '-W', '-f=${Package} ${Version}
> ${Status}\n'], 'dpkg-packages')
>
> -    try:
> -        load_plugins()
> -    except:
> -        pass
> -
> +    # Filter out ovs related information if --ovs option passed
> +    if only_ovs_info:
> +        ovs_info_caps = [CAP_NETWORK_STATUS, CAP_SYSTEM_LOGS,
> +                         CAP_NETWORK_CONFIG]
> +        ovs_info_list = ['process-tree']
> +        for (k, v) in data.items():
> +            cap = v['cap']
> +            ovs_info = k[0] if v.has_key('filename') else k
> +            if ovs_info not in ovs_info_list and cap not in ovs_info_caps:
> +                del data[k]
>

Some suggestions, just wrote them inline as comments or by changing the
line.
 - maybe comment on not using iteritems
 - 'in' preferred over 'has_key'
 - pythonisms from 2.6 might not be supported

# We cannot use iteritems, since we modify 'data' as we pass through
for (k, v) in data.items():
    cap = v['cap']
    # if __ else __ isn't supported in python 2.4, which Xen might still use
    # could still do a 1-liner like
    # info = (('filename' in v) and [k[0]] or [k])[0]
    if 'filename' in v:
        info = k[0]
    else:
        info = k
    if info not in ovs_info_list and cap not in ovs_info_caps:
        del data[k]



> +
> +        load_plugins(False, 'ovs')


python style wants load_plugins(just_capabilities=False, filter='ovs')
I'm unsure why you explicitly mark just_capabilities False here, but leave
it
to be determined by the default below.

This changes the behavior - exceptions from load_plugins(False, 'ovs') will
be raised instead of caught and ignored (as below).  Is this deliberate?


> +    else:
> +       try:
> +           load_plugins()
> +       except:
> +           pass
> +
>      # permit the user to filter out data
> -    for k in sorted(data.keys()):
> -        if not ANSWER_YES_TO_ALL and not yes("Include '%s'? [Y/n]: " % k):
> -            del data[k]
> +    for (k, v) in sorted(data.items()):


I don't know what load_plugins does, but it seems like you might
be able to merge these two iterations through data, if it doesn't effect
things.


> +       cap = v['cap']
> +       key = k[0] if v.has_key('filename') else k
>

see comment above about the identical line


> +       if not ANSWER_YES_TO_ALL and not yes("Include '%s'? [Y/n]: " %
> key):
> +           del data[k]
>
>      # collect selected data now
>      output_ts('Running commands to collect data')
> @@ -773,7 +800,7 @@ def module_info(cap):
>
>
>  def multipathd_topology(cap):
> -    pipe = Popen([MULTIPATHD, '-k'], bufsize=1, stdin=PIPE,
> +    pipe = Popen([MULTIPATHD, '-k'], bufsize=1, stdin=PIPE,
>                       stdout=PIPE, stderr=dev_null)
>      stdout, stderr = pipe.communicate('show topology')
>
> @@ -837,7 +864,7 @@ def dump_rdac_groups(cap):
>                  group, _ = line.split(None, 1)
>                  cmd_output(cap, [MPPUTIL, '-g', group])
>
> -def load_plugins(just_capabilities = False):
> +def load_plugins(just_capabilities = False, filter = None):
>

python style prefers no spaces around '=' here, though I see existing code
had it that way.


>      def getText(nodelist):
>          rc = ""
>          for node in nodelist:
> @@ -851,7 +878,7 @@ def load_plugins(just_capabilities = False):
>          if val in ['true', 'false', 'yes', 'no']:
>              ret = val in ['true', 'yes']
>          return ret
> -
> +
>      for dir in [d for d in os.listdir(PLUGIN_DIR) if
> os.path.isdir(os.path.join(PLUGIN_DIR, d))]:
>          if not caps.has_key(dir):
>              if not os.path.exists("%s/%s.xml" % (PLUGIN_DIR, dir)):
> @@ -881,28 +908,34 @@ def load_plugins(just_capabilities = False):
>
>          if just_capabilities:
>              continue
> -
> +
>          plugdir = os.path.join(PLUGIN_DIR, dir)
>          for file in [f for f in os.listdir(plugdir) if
> f.endswith('.xml')]:
>              xmldoc = parse(os.path.join(plugdir, file))
>              assert xmldoc.documentElement.tagName == "collect"
>
>              for el in xmldoc.documentElement.getElementsByTagName("*"):
> +                filters_tmp = el.getAttribute("filters")
> +                filters = [] if filters_tmp == '' else
> filters_tmp.split(',')
>

More post 2.5-ism, perhaps

  filters = filters_tmp and filters_tmp.split(',') or []

                 if el.tagName == "files":
>                      newest_first = getBoolAttr(el, 'newest_first')
> -                    file_output(dir, getText(el.childNodes).split(),
> -                                newest_first=newest_first)
> +                    if filter == None or filter in filters:
>

style prefers 'is None'

  if filter is None or filter in filters:

same for below.

+                        file_output(dir, getText(el.childNodes).split(),
> +                                    newest_first=newest_first)

                 elif el.tagName == "directory":
>                      pattern = el.getAttribute("pattern")
>                      if pattern == '': pattern = None
>                      negate = getBoolAttr(el, 'negate')
>                      newest_first = getBoolAttr(el, 'newest_first')
> -                    tree_output(dir, getText(el.childNodes), pattern and
> re.compile(pattern) or None,
> -                                negate=negate, newest_first=newest_first)
> +                    if filter == None or filter in filters:
> +                        tree_output(dir, getText(el.childNodes),
> +                                    pattern and re.compile(pattern) or
> None,
> +                                    negate=negate,
> newest_first=newest_first)
>                  elif el.tagName == "command":
>                      label = el.getAttribute("label")
>                      if label == '': label = None
> -                    cmd_output(dir, getText(el.childNodes), label)
> +                    if filter == None or filter in filters:
> +                        cmd_output(dir, getText(el.childNodes), label)
>

It seems like you could move the filter to above the if/elif blocks to avoid
having to get the attributes at all, since you don't use any of that
information
to make the filtering choice.  Is that possible?


Seems reasonable overall.

  -Reid
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20120710/0e3b9bde/attachment-0003.html>


More information about the dev mailing list