A few comments inline
On Tue, Jul 10, 2012 at 12:07 PM, Arun Sharma <arun.sha...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
