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

Arun Sharma arun.sharma at calsoftinc.com
Thu Jul 12 09:41:45 UTC 2012


Reworked and its present in v3. Also updated ovs-bugtool manual file.
Comments inline.

From:  Reid Price <reid at nicira.com>
Date:  Wed, 11 Jul 2012 12:00:02 -0700
To:  Arun Sharma <arun.sharma at calsoftinc.com>
Cc:  <dev at openvswitch.org>
Subject:  Re: [ovs-dev] [PATCH v2] ovs-bugtool: Added --ovs option to get
only ovs related information

Thanks for the fixes!  A couple nitpick comments inline.  v2  and your
responses in v1 resolved all of my previous concerns.

On Tue, Jul 10, 2012 at 11:40 PM, Arun Sharma <arun.sharma at calsoftinc.com>
wrote:
> +    # Filter out ovs relevant information if --ovs option passed
> +    # else collect all information
> +    if only_ovs_info:
> +        ovs_info_caps = [CAP_NETWORK_STATUS, CAP_SYSTEM_LOGS,
> +                         CAP_NETWORK_CONFIG]
> +        ovs_info_list = ['process-tree']
> +        # We cannot use iteritems, since we modify 'data' as we pass through
> +        for (k, v) in data.items():
> +            cap = v['cap']
> +            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]
> +
> +        try:
> +            load_plugins(filter='ovs')
> +        except:
> +            pass
> +    else:
> +       try:
> +           load_plugins()
> +       except:
> +           pass
> +

This probably will be easier to maintain by having the
try/except outside of the if/else.

  filters = set()
  if only_ovs_info:
      filters.add('ovs')
      ...
  filter = ",".join(filters)
  try:
      load_plugins(filter=filter)
  except:
      pass
 

[Arun] - Implemented it as below.
             It requires it to have "filter = None" in else clause. Since in
load_plugins function, condition "if not(filter is None or filter in
filters):  continue" will be always true when filter=''.

    if len(filters):
        filter = ",".join(filters)
    else:
        filter = None

     try:
        load_plugins(filter=filter)
     except:
         pass


 
>              for el in xmldoc.documentElement.getElementsByTagName("*"):
> -                if el.tagName == "files":
> -                    newest_first = getBoolAttr(el, 'newest_first')
> -                    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)
> -                elif el.tagName == "command":
> -                    label = el.getAttribute("label")
> -                    if label == '': label = None
> -                    cmd_output(dir, getText(el.childNodes), label)
> +                filters_tmp = el.getAttribute("filters")
> +                if filters_tmp == '':
> +                    filters = []
> +                else:
> +                    filters = filters_tmp.split(',')
> +                if filter is None or filter in filters:

Nit, you can save this whole level of indentation by doing

  if not(filter is None or filter in filters):  continue

[Arun] - Done
 
> +                    if el.tagName == "files":
> +                        newest_first = getBoolAttr(el, 'newest_first')
> +                        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)
> +                    elif el.tagName == "command":
> +                        label = el.getAttribute("label")
> +                        if label == '': label = None
> +                        cmd_output(dir, getText(el.childNodes), label)

Looks good to me otherwise.

  -Reid 


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20120712/988f453d/attachment-0003.html>


More information about the dev mailing list