Thanks for the review comments. Applied the comments and the comments are mentioned inline.
I will share Patch V2 in next email. From: Reid Price <r...@nicira.com> Date: Tue, 10 Jul 2012 18:57:12 -0700 To: Arun Sharma <arun.sha...@calsoftinc.com> Cc: <dev@openvswitch.org> Subject: Re: [ovs-dev] [PATCH] ovs-bugtool: Added --ovs option to get only ovs related information 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 <http://ovs-bugtool.in> > b/utilities/bugtool/ovs-bugtool.in <http://ovs-bugtool.in> > index 3bafa13..af2bb6e 100755 > --- a/utilities/bugtool/ovs-bugtool.in <http://ovs-bugtool.in> > +++ b/utilities/bugtool/ovs-bugtool.in <http://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? [Arun] - Its not redundant. Basically the intention was to keep another variable which will say "collect all information or not." Variable name 'filter_data' was kept to identify is there any kind of filtering required or not. If 'filter_data' is False then bug tool should collect all info. To make it more meaningful, i have renamed this variable to 'collect_all_info'. > @@ -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)'? [Arun] - Same comment apply as above for 'filter_data' which is renamed now to 'collect_all_info'. The intention was to collect the information if "no" filtering is asked or "only ovs info" is asked. After renaming the variable, the condition is more meaningful and self explanatory - if collect_all_info 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] [Arun] - Done. > + > + 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. [Arun] - Done. I think i can call as below and just_capabilities remains default False as expected. load_plugins(filter='ovs') This changes the behavior - exceptions from load_plugins(False, 'ovs') will be raised instead of caught and ignored (as below). Is this deliberate? [Arun] - Oops, missed out. It was not intentional. > + 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. [Arun] - It will effect the things if we try to merge. load_plugins adds key:value in 'data' which is basically adding rules to set how to collect information (could be command output, filename, function returns output). In first iteration, which is inside condition (if only_ovs_info), it deletes some elements from 'data' based on the filtering condition. Then in second iteration called after load_plugins is called in any of the above conditions (with or without filtering). The second iteration used to prompts user, if they want to perform another level of filtering by pressing (Y/N) for each data element to collect. > + cap = v['cap'] > + key = k[0] if v.has_key('filename') else k see comment above about the identical line [Arun] - Done > + 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. [Arun] - Done. True, there shouldn't be any space. Initially I thought to make it consistent with current. But after reviewing the code, it was observed in most places that it does not uses spaces and in few places it use spaces. Removed the spaces now. > 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 [Arun] - Done, Used as below if filters_tmp == '': filters = [] else: filters = filters_tmp.split(',') 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. [Arun] - Done > + 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? [Arun] - I hope here you mean, to keep condition "if filter is None or filter in filters:" above the if/elif block which is actually common for all three different kind of elements "files, directory, command". This will help to avoid fetching attributes of those elements which it will never use if the condition becomes false. If this is what you mean? i have fixed this. Seems reasonable overall. -Reid
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
