Thanks for the review. Please see inlines. Shih-Hao
----- Original Message ----- > From: "Gurucharan Shetty" <[email protected]> > To: "Shih-Hao Li" <[email protected]> > Cc: "dev" <[email protected]>, "Shih-Hao Li" <[email protected]> > Sent: Friday, September 27, 2013 10:15:36 AM > Subject: Re: [ovs-dev] [PATCH] ovs-bugtool: Change log-days parameter based > on file last_mod_time > On Thu, Sep 26, 2013 at 1:25 PM, Shih-Hao Li <[email protected]> wrote: > > From: Shih-Hao Li <[email protected]> > > > > Previously the log-days parameter can only support rotated logs > > based on their numbered filename extension. Thus it can not be > > used for other types of log filenames. This patch changes it to > > be based on file last modification time. > > > > Issue: #19671 > You need a Signed-off-by line here. I will add this. > > --- > > utilities/bugtool/ovs-bugtool.8.in | 6 ++-- > > utilities/bugtool/ovs-bugtool.in | 67 +++++++++++++++++++++++++----------- > > 2 files changed, 49 insertions(+), 24 deletions(-) > > > > diff --git a/utilities/bugtool/ovs-bugtool.8.in > > b/utilities/bugtool/ovs-bugtool.8.in > > index e353604..6f4e0b5 100644 > > --- a/utilities/bugtool/ovs-bugtool.8.in > > +++ b/utilities/bugtool/ovs-bugtool.8.in > > @@ -33,9 +33,9 @@ Print verbose debugging output. > > Use the capabilities specified in a comma-separated list. > > . > > .IP "\fB\-\-log\-days=\fIdays\fR" > > -Include the logs rotated in the previous \fIdays\fR days in the debug > > bundle. > > -The number of log files included has a big impact on the eventual bundle > > size. > > -The default value is 20 days. > > +Include the logs with last modification time in the previous \fIdays\fR > > days > > +in the debug bundle. The number of log files included has a big impact on > > the > > +eventual bundle size. The default value is 20 days. > > . > > .IP "\fB\-\-output=\fIfiletype\fR" > > Generate a debug bundle with the specified file type. Options include > > diff --git a/utilities/bugtool/ovs-bugtool.in > > b/utilities/bugtool/ovs-bugtool.in > > index baa0af7..b2172c8 100755 > > --- a/utilities/bugtool/ovs-bugtool.in > > +++ b/utilities/bugtool/ovs-bugtool.in > > @@ -222,6 +222,7 @@ unlimited_data = False > > dbg = False > > # Default value for the number of rotated logs. > > log_days = 20 > > +log_min_time = None > > free_disk_space = None > > > > def cap(key, pii=PII_MAYBE, min_size=-1, max_size=-1, min_time=-1, > > @@ -286,7 +287,8 @@ def cmd_output(cap, args, label=None, filter=None, > > binary=False): > > 'binary': binary} > > > > > > -def file_output(cap, path_list, newest_first=False): > > +def file_output(cap, path_list, newest_first=False, min_time=None, > > + max_time=None): > I see max_time as an option. We don't seem to be using it for any > particular feature. If so, may be we can get rid of it. I added it to be paired with max_time, like the min_time and max_time in capability. They are not used now. Just for potential use in the future. > > """ > > If newest_first is True, the list of files in path_list is sorted > > by file modification time in descending order, else its sorted > > @@ -299,6 +301,10 @@ def file_output(cap, path_list, newest_first=False): > > s = os.stat(path) > > except OSError, e: > > continue > > + if min_time is not None and s.st_mtime < min_time: > > + continue > > + if max_time is not None and s.st_mtime > max_time: > > + continue > > path_entries.append((path, s)) > > > > mtime = lambda(path, stat): stat.st_mtime > > @@ -308,7 +314,8 @@ def file_output(cap, path_list, newest_first=False): > > data[p] = {'cap': cap, 'filename': p[0]} > > > > > > -def tree_output(cap, path, pattern=None, negate=False, > > newest_first=False): > > +def tree_output(cap, path, pattern=None, negate=False, newest_first=False, > > + min_time=None, max_time=None): > > """ > > Walks the directory tree rooted at path. Files in current dir are processed > > before files in sub-dirs. > > @@ -318,23 +325,27 @@ def tree_output(cap, path, pattern=None, > > negate=False, newest_first=False): > > for root, dirs, files in os.walk(path): > > fns = [fn for fn in [os.path.join(root, f) for f in files] > > if os.path.isfile(fn) and matches(fn, pattern, negate)] > > - file_output(cap, fns, newest_first=newest_first) > > + file_output(cap, fns, newest_first=newest_first, > > + min_time=min_time, max_time=max_time) > > + > > + > > +def prefix_output(cap, prefix, newest_first=False, min_time=None, > > max_time=None): > > + """ > > + Output files with the same prefix. > > + """ > > + fns = [] > > + for root, dirs, files in os.walk(os.path.dirname(prefix)): > > + fns += [fn for fn in [os.path.join(root, f) for f in files] > > + if fn.startswith(prefix)] > > + file_output(cap, fns, newest_first=newest_first, > > + min_time=min_time, max_time=max_time) > > + > > > > def func_output(cap, label, func): > > if cap in entries: > > t = str(func).split() > > data[label] = {'cap': cap, 'func': func} > > > > -def log_output(cap, logs, newest_first=False): > > - global log_days > > - file_output(cap, logs) > > - file_output(cap, > > - ['%s.%d' % (f, n) for n in range(1, log_days+1) for f in logs], \ > > - newest_first=newest_first) > > - file_output(cap, > > - ['%s.%d.gz' % (f, n) for n in range(1, log_days+1) for f in logs], \ > > - newest_first=newest_first) > > - > > def collect_data(): > > process_lists = {} > > > > @@ -370,7 +381,8 @@ def collect_data(): > > > > def main(argv=None): > > global ANSWER_YES_TO_ALL, SILENT_MODE > > - global entries, data, dbg, unlimited_data, log_days, free_disk_space > > + global entries, data, dbg, unlimited_data, free_disk_space > > + global log_days, log_min_time > > > > # Filter flags > > only_ovs_info = False > > @@ -473,6 +485,8 @@ def main(argv=None): > > if output_file is not None and not unlimited_data: > > free_disk_space = get_free_disk_space(output_file) * 90 / 100 > > > > + log_min_time = int(time.time()) - log_days * 86400 > > + > > if ANSWER_YES_TO_ALL: > > output("Warning: '--yestoall' argument provided, will not prompt for > > individual files.") > > > > @@ -585,11 +599,14 @@ exclude those logs from the archive. > > system_logs = ([ VAR_LOG_DIR + x for x in > > ['crit.log', 'kern.log', 'daemon.log', 'user.log', > > 'syslog', 'messages', 'secure', 'debug', 'dmesg', 'boot']]) > > + for log in system_logs: > > + prefix_output(CAP_SYSTEM_LOGS, log, min_time=log_min_time) > > + > > ovs_logs = ([ OPENVSWITCH_LOG_DIR + x for x in > > ['ovs-vswitchd.log', 'ovsdb-server.log', > > 'ovs-xapi-sync.log', 'ovs-monitor-ipsec.log', 'ovs-ctl.log']]) > > - log_output(CAP_SYSTEM_LOGS, system_logs) > > - log_output(CAP_OPENVSWITCH_LOGS, ovs_logs) > > + for log in ovs_logs: > > + prefix_output(CAP_OPENVSWITCH_LOGS, log, min_time=log_min_time) > > > > if not os.path.exists('/var/log/dmesg') and not > > os.path.exists('/var/log/boot'): > > cmd_output(CAP_SYSTEM_LOGS, [DMESG]) > > @@ -808,6 +825,7 @@ def dump_rdac_groups(cap): > > cmd_output(cap, [MPPUTIL, '-g', group]) > > > > def load_plugins(just_capabilities=False, filter=None): > > + global log_min_time > > def getText(nodelist): > > rc = "" > > for node in nodelist: > > @@ -868,8 +886,9 @@ def load_plugins(just_capabilities=False, filter=None): > > if el.tagName == "files": > > newest_first = getBoolAttr(el, 'newest_first') > > if el.getAttribute("type") == "logs": > > - log_output(dir, getText(el.childNodes).split(), > > - newest_first=newest_first) > > + file_output(dir, getText(el.childNodes).split(), > > + newest_first=newest_first, > > + min_time=log_min_time) > I think this should be prefix_output() instead of file_output(). > Otherwise the rotated logs won't be picked up. Good catch. But I am not sure if users expect the filenames listed under <files> to be exact-match or not. My preference is to use "type=logs" only for those files or directories that need to be controlled by "--log-days" parameter. If users want to use wildcard for rotated logs, they can define that in "pattern" under <directory>, such as <directory pattern = "messages.*" > /var/log</directory> What do you think? > > else: > > file_output(dir, getText(el.childNodes).split(), > > newest_first=newest_first) > > @@ -878,9 +897,15 @@ def load_plugins(just_capabilities=False, > > filter=None): > > 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 el.getAttribute("type") == "logs": > > + tree_output(dir, getText(el.childNodes), > > + pattern and re.compile(pattern) or None, > > + negate=negate, newest_first=newest_first, > > + min_time=log_min_time) > > + else: > > + 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 > > -- > > 1.7.9.5 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > http://openvswitch.org/mailman/listinfo/dev
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
