[ovs-dev] [PATCH] ovs-bugtool: Add ability to prioritize files by date.

[Reid Price]

Contextless notes inline

On Mar 23, 2555 BE, at 7:10, Raju Subramanian <rsubramanian at nicira.com> wrote:

> When size limit is reached in the middle of processing a dir,
> the report ends up containing oldest files. This change adds
> an optional param in the plugin to prioritize newer files.
> 
> Feature #9937
> Requested-by: Ronald Lee <rlee at nicira.com>
> Signed-off-by: Raju Subramanian <rsubramanian at nicira.com>
> ---
> AUTHORS                          |    1 +
> utilities/bugtool/ovs-bugtool.in |   61 +++++++++++++++++++++++++------------
> 2 files changed, 42 insertions(+), 20 deletions(-)
> 
> diff --git a/AUTHORS b/AUTHORS
> index fa47e50..92aa47b 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -39,6 +39,7 @@ Neil McKee              neil.mckee at inmon.com
> Paul Fazzone            pfazzone at nicira.com
> Philippe Jung           phil.jung at free.fr
> Pravin B Shelar         pshelar at nicira.com
> +Raju Subramanian        rsubramanian at nicira.com
> Ravi Kerur              Ravi.Kerur at telekom.com
> Reid Price              reid at nicira.com
> Rob Hoes                rob.hoes at citrix.com
> diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in
> index f78289d..cc7433e 100755
> --- a/utilities/bugtool/ovs-bugtool.in
> +++ b/utilities/bugtool/ovs-bugtool.in
> @@ -319,30 +319,51 @@ def cmd_output(cap, args, label = None, filter = None):
>                 label = args
>         data[label] = {'cap': cap, 'cmd_args': args, 'filter': filter}
> 
> -def file_output(cap, path_list):
> +def fcmp(x, y):
> +    """Compares the st_mtime attribute for the two file entries.
> +
> +    x and y are expected to be tuples where the second element in
> +    the tuple is the return value from os.stat.
> +    """
> +    x_st_mtime = x[1].st_mtime
> +    y_st_mtime = y[1].st_mtime
> +    if x_st_mtime == y_st_mtime:
> +        return 0
> +    elif x_st_mtime > y_st_mtime:
> +        return 1
> +    return -1
Function won't be necessary per below

> +
> +def file_output(cap, path_list, newest_first = False):
> +    """
> +    If newest_first is True, the list of files in path_list is sorted
> +    by file modification time in descending order, else its sorted
> +    in ascending order.
> +    """
>     if cap in entries:
Cap seems a bit overloaded, wasn't sure whether it meant max size allowed or capture. Context undoubtably makes this obvious

> -        for p in path_list:
> -            if os.path.exists(p):
> -                if unlimited_data or caps[cap][MAX_SIZE] == -1 or \
> -                        cap_sizes[cap] < caps[cap][MAX_SIZE]:
> -                    data[p] = {'cap': cap, 'filename': p}
> -                    try:
> -                        s = os.stat(p)
> -                        cap_sizes[cap] += s.st_size
> -                    except:
> -                        pass
> -                else:
> -                    output("Omitting %s, size constraint of %s exceeded" % (p, cap))
> +        path_entries = [(path, os.stat(path)) for path in path_list if os.path.exists(path)]
Does this exceed 80?

> +        path_entries.sort(cmp=fcmp, reverse=newest_first)
To follow on Ben's note, a preferred version would probably be

 mtime = lambda (path, stat): stat.st_mtime
 for p in sorted(path_entries, key=mtime):
> +        for p in path_entries:

> +            if unlimited_data or caps[cap][MAX_SIZE] == -1 or \
> +                    cap_sizes[cap] < caps[cap][MAX_SIZE]:
> +                data[p] = {'cap': cap, 'filename': p[0]}
> +                try:
> +                    cap_sizes[cap] += p[1].st_size
> +                except:
Why the unguarded except?  This is asking for all kinds of trouble

> +                    pass
> +            else:
> +                output("Omitting %s, size constraint of %s exceeded" % (p[0], cap))
> 
> -def tree_output(cap, path, pattern = None, negate = False):
> +def tree_output(cap, path, pattern = None, negate = False, newest_first = False):
Not your trend, but style disapproves of spaces around default arguments here.

> +    """
> +    Walks the directory tree rooted at path. Files in current dir are processed
> +    before files in sub-dirs.
> +    """
>     if cap in entries:
>         if os.path.exists(path):
> -            for f in os.listdir(path):
> -                fn = os.path.join(path, f)
> -                if os.path.isfile(fn) and matches(fn, pattern, negate):
> -                    file_output(cap, [fn])
> -                elif os.path.isdir(fn):
> -                    tree_output(cap, fn, pattern, negate)
> +            for root, dirs, files in os.walk(path):
> +                fns = [f for f in [os.path.join(root, f) for f in files] \
> +                           if os.path.isfile(f) and matches(f, pattern, negate)]
Duplicate use of 'f' makes me nervous here.  Seems like precipitous code, to make up a phrase.

> +                file_output(cap, fns, newest_first)
Style nit, use newest_first=newest_first (as with any kwarg, to guard future position changes)
> 
> def func_output(cap, label, func):
>     if cap in entries:

I've forgotten bugtool's structure/config.  I assume this has logic elsewhere to split up the size quota into smaller chunks, so the global max couldn't be taken up by the first giant file it comes to.

Again, sorry for any typos or lack of context, composed via phone.

  -Reid

> -- 
> 1.7.2.5
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev