On 12/08/2015 01:59 PM, Ben Pfaff wrote:
> Requested-by: P R Dinesh
> Requested-at: https://github.com/openvswitch/ovs/pull/94
> Signed-off-by: Ben Pfaff <b...@ovn.org>
Acked-by: Russell Bryant <russ...@ovn.org>
Some comments/questions inline ...
> ---
> lib/vlog.c | 24 ++++++++++++++++++
> python/ovs/vlog.py | 11 +++++++-
> tests/vlog.at | 74
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 108 insertions(+), 1 deletion(-)
>
> diff --git a/lib/vlog.c b/lib/vlog.c
> index 28cea5d..2b8ba2b 100644
> --- a/lib/vlog.c
> +++ b/lib/vlog.c
> @@ -677,6 +677,28 @@ vlog_unixctl_reopen(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> }
>
> static void
> +vlog_unixctl_close(struct unixctl_conn *conn, int argc OVS_UNUSED,
> + const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +{
> + ovs_mutex_lock(&log_file_mutex);
> + if (log_fd >= 0) {
> + close(log_fd);
> + log_fd = -1;
> +
> + async_append_destroy(log_writer);
> + log_writer = NULL;
> +
> + struct vlog_module *mp;
> + LIST_FOR_EACH (mp, list, &vlog_modules) {
> + update_min_level(mp);
> + }
> + }
> + ovs_mutex_unlock(&log_file_mutex);
This got me looking around at where log_file_mutex is used. One thing I
noticed was that it seems like it should be locked in
vlog_insert_module(). In pratice it doesn't matter since it's only
called in global constructors at startup. Maybe it makes sense in case
someone decides to call vlog_insert_module() dynamically for some reason.
If we do that, maybe the lock should be held in vlog_module_from_name(),
as well.
Again, not a real issue today, but I'm curious what you think ...
> diff --git a/tests/vlog.at b/tests/vlog.at
> index a614647..269bfc5 100644
> --- a/tests/vlog.at
> +++ b/tests/vlog.at
> @@ -255,6 +255,80 @@ AT_CHECK([sed 's/.*|//' log], [0], [dnl
> ])
> AT_CLEANUP
>
> +AT_SETUP([vlog - vlog/close - C])
> +on_exit 'kill `cat test-unixctl.pid`'
> +
> +AT_CAPTURE_FILE([log])
> +AT_CAPTURE_FILE([log.old])
> +AT_CHECK([ovstest test-unixctl --log-file=`pwd`/log --pidfile --detach],
> + [0], [], [stderr])
> +AT_CHECK([vlog_filt stderr], [0], [opened log file
> +])
> +
> +AT_CHECK([APPCTL -t test-unixctl log message])
> +AT_CHECK([APPCTL -t test-unixctl log message2])
> +
> +# After closing the log file, message3 won't appear anywhere.
> +AT_CHECK([APPCTL -t test-unixctl vlog/close])
> +mv log log.old
> +AT_CHECK([APPCTL -t test-unixctl log message3])
> +
> +# Closing the log file again is harmless.
> +AT_CHECK([APPCTL -t test-unixctl vlog/close])
> +AT_CHECK([APPCTL -t test-unixctl log message4])
> +
> +# After reopening the log file, further messages start appearing again.
> +AT_CHECK([APPCTL -t test-unixctl vlog/reopen])
> +AT_CHECK([APPCTL -t test-unixctl log message5])
> +AT_CHECK([APPCTL -t test-unixctl exit])
> +
> +AT_CHECK([vlog_filt log.old], [0], [dnl
> +opened log file
> +Entering run loop.
> +message
> +message2
> +])
> +AT_CHECK([vlog_filt log], [0], [dnl
> +opened log file
> +message5
> +])
> +AT_CLEANUP
> +
> +AT_SETUP([vlog - vlog/close - Python])
> +AT_SKIP_IF([test $HAVE_PYTHON = no])
> +on_exit 'kill `cat test-unixctl.py.pid`'
> +
> +AT_CAPTURE_FILE([log])
> +AT_CAPTURE_FILE([log.old])
> +AT_CHECK([$PYTHON $srcdir/test-unixctl.py --log-file=`pwd`/log --pidfile
> --detach])
> +
> +AT_CHECK([APPCTL -t test-unixctl.py log message])
> +AT_CHECK([APPCTL -t test-unixctl.py log message2])
> +
> +# After closing the log file, message3 won't appear anywhere.
> +AT_CHECK([APPCTL -t test-unixctl.py vlog/close])
> +mv log log.old
> +AT_CHECK([APPCTL -t test-unixctl.py log message3])
> +
> +# Closing the log file again is harmless.
> +AT_CHECK([APPCTL -t test-unixctl.py vlog/close])
> +AT_CHECK([APPCTL -t test-unixctl.py log message4])
> +
> +# After reopening the log file, further messages start appearing again.
> +AT_CHECK([APPCTL -t test-unixctl.py vlog/reopen])
> +AT_CHECK([APPCTL -t test-unixctl.py log message5])
> +AT_CHECK([APPCTL -t test-unixctl.py exit])
> +
> +AT_CHECK([sed 's/.*|//' log.old], [0], [dnl
> + Entering run loop.
> + message
> + message2
> +])
> +AT_CHECK([sed 's/.*|//' log], [0], [dnl
> + message5
> +])
> +AT_CLEANUP
> +
> AT_SETUP([vlog - vlog/set and vlog/list - C])
> on_exit 'kill `cat test-unixctl.pid`'
The two tests are *very* similar. It'd be nice to share most of this as
is done in other parts of the test suit, but it's not that big of a
deal. The minor differences might make it not worth the trouble.
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
