Contextless notes inline
On Mar 23, 2555 BE, at 7:10, Raju Subramanian <rsubraman...@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 <r...@nicira.com>
> Signed-off-by: Raju Subramanian <rsubraman...@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.mc...@inmon.com
> Paul Fazzone pfazz...@nicira.com
> Philippe Jung phil.j...@free.fr
> Pravin B Shelar pshe...@nicira.com
> +Raju Subramanian rsubraman...@nicira.com
> Ravi Kerur ravi.ke...@telekom.com
> Reid Price r...@nicira.com
> Rob Hoes rob.h...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
