I sent out a revised version:
http://openvswitch.org/pipermail/dev/2013-December/034644.html
Will you look at it?
On Tue, Dec 03, 2013 at 09:59:19PM -0800, Henry Mai wrote:
> Sorry, found another violation where I had an extra newline before a function.
>
> On Tue, Dec 3, 2013 at 8:30 PM, Henry Mai <henry...@nicira.com> wrote:
> > Another patch to address style issues
> >
> > On Tue, Dec 3, 2013 at 3:19 PM, Henry Mai <henry...@nicira.com> wrote:
> >> Here's an updated patch.
> >>
> >> On Mon, Dec 2, 2013 at 2:06 PM, Ben Pfaff <b...@nicira.com> wrote:
> >>> On Mon, Nov 25, 2013 at 11:08:15PM -0800, Henry Mai wrote:
> >>>> Last patch got sent word wrapped, trying again.
> >>>
> >>>> This change allows vlog to export to a specified local udp syslog sink.
> >>>>
> >>>> Signed-off-by: Henry Mai <henry...@nicira.com>
> >>>
> >>> "sparse" says:
> >>>
> >>> ../lib/vlog.c:887:10: warning: incorrect type in argument 5
> >>> (different base types)
> >>> ../lib/vlog.c:887:10: expected struct sockaddr const *<noident>
> >>> ../lib/vlog.c:887:10: got struct sockaddr_in *<noident>
> >>>
> >>> lib/vlog.man should document the new option.
> >>>
> >>> Lots of stuff mentioned in CodingStyle should be changed:
> >>>
> >>> - Use /**/ not // for comments.
> >>>
> >>> - Write comments as complete sentences that end in periods.
> >>>
> >>> - Each function, and each variable declared outside a function
> >>> should be preceded by a comment.
> >>>
> >>> - Put one blank line between top-level definitions of
> >>> functions and global variables.
> >>>
> >>> - Put the return type, function name, and the braces that
> >>> surround the function's code on separate lines, all starting
> >>> in column 0.
> >>>
> >>> Please group the new static vars together rather than putting a
> >>> prototype between them.
> >>>
> >>> This looks odd in our tree:
> >>> if (!error) {
> >>> loopback_mtu_size = mtu;
> >>> } // else it's just the default (1500)
> >>> I'd be inclined to write it as:
> >>> if (!error) {
> >>> loopback_mtu_size = mtu;
> >>> } else {
> >>> /* Else it's just the default (1500). */
> >>> }
> >>> or just:
> >>> loopback_mtu_size = !error ? mtu : 1500;
> >>>
> >>> Actually here's how I'd probably write the whole thing (you forgot to
> >>> close the netdev, by the way):
> >>> error = netdev_open("lo", "system", &netdev);
> >>> if (!error) {
> >>> error = netdev_get_mtu(netdev, &mtu);
> >>> netdev_close(netdev);
> >>> }
> >>> loopback_mtu_size = !error ? mtu : 1500;
> >>>
> >>> This code looks expensive due to the big memset:
> >>> memset(hostname, 0, 255);
> >>> gethostname(hostname, 255);
> >>> ds_put_cstr(s, hostname);
> >>> I know that gethostname() might not null-terminate if the hostname is
> >>> too long but I think that the following is sufficent:
> >>> gethostname(hostname, sizeof hostname);
> >>> hostname[sizeof hostname - 1] = '\0';
> >>> ds_put_cstr(s, hostname);
> >>>
> >>> I am a little nervous about assuming that LOG_* has the same values
> >>> required by RFC 5424. I guess that the values will be correct for all
> >>> Unix-like systems, but I am not sure that this assumption is warranted
> >>> generally.
> >>>
> >>> This is indented funny:
> >>> + sendto(
> >>> + syslog_sink_fd,
> >>> + ds_cstr_ro(syslog_message),
> >>> + MIN(syslog_message->length, max_payload_size),
> >>> + 0,
> >>> + &ipaddr,
> >>> + sizeof ipaddr);
> >>> I would write it as:
> >>> sendto(syslog_sink_fd, ds_cstr_ro(syslog_message),
> >>> MIN(syslog_message->length, max_payload_size), 0,
> >>> &ipaddr, sizeof ipaddr);
> >>>
> >>> In vlog_valist(), it seems wasteful to format the message even if the
> >>> formatted message will not be used (the common case).
> >>>
> >>> Thanks,
> >>>
> >>> Ben.
> This change allows vlog to export to a specified local udp syslog sink.
>
> Signed-off-by: Henry Mai <henry...@nicira.com>
> ---
> NEWS | 1 +
> lib/vlog.c | 114
> ++++++++++++++++++++++++++++++++++++++++++++--
> lib/vlog.h | 26 ++++++++---
> lib/vlog.man | 4 ++
> utilities/ovs-appctl.8.in | 6 +++
> 5 files changed, 141 insertions(+), 10 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 38e3d9d..ede9338 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -23,6 +23,7 @@ Post-v2.0.0
> packaged or installed by default, because too many users assumed
> incorrectly that ovs-controller was a necessary or desirable part
> of an Open vSwitch deployment.
> + - Added vlog option to export to a local udp syslog sink.
>
>
> v2.0.0 - 15 Oct 2013
> diff --git a/lib/vlog.c b/lib/vlog.c
> index b1ca158..61d427d 100644
> --- a/lib/vlog.c
> +++ b/lib/vlog.c
> @@ -23,6 +23,7 @@
> #include <stdarg.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <sys/socket.h>
> #include <sys/stat.h>
> #include <sys/types.h>
> #include <syslog.h>
> @@ -32,8 +33,11 @@
> #include "coverage.h"
> #include "dirs.h"
> #include "dynamic-string.h"
> +#include "netdev.h"
> #include "ofpbuf.h"
> +#include "ovs-atomic.h"
> #include "ovs-thread.h"
> +#include "packets.h"
> #include "sat-math.h"
> #include "svec.h"
> #include "timeval.h"
> @@ -111,11 +115,29 @@ static int log_fd OVS_GUARDED_BY(log_file_mutex) = -1;
> static struct async_append *log_writer OVS_GUARDED_BY(log_file_mutex);
> static bool log_async OVS_GUARDED_BY(log_file_mutex);
>
> +/* Syslog export configuration. */
> +static atomic_int udp_syslog_target_port = ATOMIC_VAR_INIT(0);
> +static int loopback_mtu_size = 1500;
> +static int syslog_sink_fd = -1;
> +
> static void format_log_message(const struct vlog_module *, enum vlog_level,
> enum vlog_facility,
> const char *message, va_list, struct ds *)
> PRINTF_FORMAT(4, 0) OVS_REQ_RDLOCK(&pattern_rwlock);
>
> +/* Returns the equivalent rfc 5424 severity level. */
> +static inline int
> +get_5424_level(enum vlog_level level) {
> + switch (level) {
> + case VLL_EMER: return 0;
> + case VLL_ERR: return 3;
> + case VLL_WARN: return 4;
> + case VLL_INFO: return 6;
> + case VLL_DBG: return 7;
> + default: return 0;
> + }
> +}
> +
> /* Searches the 'n_names' in 'names'. Returns the index of a match for
> * 'target', or 'n_names' if no name matches. */
> static size_t
> @@ -480,6 +502,17 @@ vlog_set_verbosity(const char *arg)
> }
> }
>
> +/* Set the vlog udp syslog target port. */
> +void
> +vlog_set_syslog_target(const char *target)
> +{
> + int value;
> + if (!str_to_int(target, 10, &value) || value < 0) {
> + value = 0;
> + }
> + atomic_store(&udp_syslog_target_port, value);
> +}
> +
> static void
> vlog_unixctl_set(struct unixctl_conn *conn, int argc, const char *argv[],
> void *aux OVS_UNUSED)
> @@ -582,6 +615,9 @@ vlog_init__(void)
> {
> static char *program_name_copy;
> long long int now;
> + struct netdev *netdev;
> + int error = 0;
> + int mtu = 0;
>
> /* openlog() is allowed to keep the pointer passed in, without making a
> * copy. The daemonize code sometimes frees and replaces 'program_name',
> @@ -598,6 +634,10 @@ vlog_init__(void)
> free(s);
> }
>
> + /* Ignore socket open failures, there is nothing else we can do about
> + * it. */
> + syslog_sink_fd = socket(AF_INET, SOCK_DGRAM, 0);
> +
> unixctl_command_register(
> "vlog/set", "{spec | PATTERN:facility:pattern}",
> 1, INT_MAX, vlog_unixctl_set, NULL);
> @@ -608,6 +648,13 @@ vlog_init__(void)
> 0, INT_MAX, vlog_disable_rate_limit, NULL);
> unixctl_command_register("vlog/reopen", "", 0, 0,
> vlog_unixctl_reopen, NULL);
> +
> + error = netdev_open("lo", "system", &netdev);
> + if (!error) {
> + error = netdev_get_mtu(netdev, &mtu);
> + netdev_close(netdev);
> + }
> + loopback_mtu_size = !error ? mtu : 1500;
> }
>
> /* Initializes the logging subsystem and registers its unixctl server
> @@ -703,7 +750,10 @@ format_log_message(const struct vlog_module *module,
> enum vlog_level level,
> enum vlog_facility facility,
> const char *message, va_list args_, struct ds *s)
> {
> + static const int local0_facility = 16;
> +
> char tmp[128];
> + char hostname[255];
> va_list args;
> const char *p;
>
> @@ -739,6 +789,10 @@ format_log_message(const struct vlog_module *module,
> enum vlog_level level,
> case 'A':
> ds_put_cstr(s, program_name);
> break;
> + case 'B':
> + ds_put_format(s, "%d",
> + ((local0_facility << 3) + get_5424_level(level)));
> + break;
> case 'c':
> p = fetch_braces(p, "", tmp, sizeof tmp);
> ds_put_cstr(s, vlog_get_module_name(module));
> @@ -751,6 +805,11 @@ format_log_message(const struct vlog_module *module,
> enum vlog_level level,
> p = fetch_braces(p, "%Y-%m-%d %H:%M:%S.###", tmp, sizeof tmp);
> ds_put_strftime_msec(s, tmp, time_wall_msec(), true);
> break;
> + case 'E':
> + gethostname(hostname, 255);
> + hostname[sizeof hostname - 1] = '\0';
> + ds_put_cstr(s, hostname);
> + break;
> case 'm':
> /* Format user-supplied log message and trim trailing new-lines.
> */
> length = s->length;
> @@ -804,6 +863,43 @@ format_log_message(const struct vlog_module *module,
> enum vlog_level level,
> }
> }
>
> +/* Returns whether or not udp syslog export is enabled.
> + * Sets the 'target_udp_port_out' parameter with the target udp port
> + * if enabled. */
> +static bool
> +udp_syslog_export_enabled(int *target_udp_port_out)
> +{
> + bool enabled = false;
> + int target_udp_port;
> + atomic_read(&udp_syslog_target_port, &target_udp_port);
> + enabled = ((target_udp_port > 0 ) && (syslog_sink_fd >= 0));
> + if (enabled) {
> + *target_udp_port_out = target_udp_port;
> + }
> + return enabled;
> +}
> +
> +/* Exports the given 'syslog_message' to the configured local udp syslog
> + * sink. */
> +static void
> +export_to_udp_syslog_target(struct ds *syslog_message, int target_udp_port)
> +{
> + static const int ipv4_udp_header_size = IP_HEADER_LEN + UDP_HEADER_LEN;
> + int max_payload_size;
> + struct sockaddr_in ipaddr;
> +
> + memset(&ipaddr, 0, sizeof ipaddr);
> +
> + ipaddr.sin_family = AF_INET;
> + ipaddr.sin_port = htons(target_udp_port);
> + inet_pton(AF_INET, "127.0.0.1", &ipaddr.sin_addr);
> +
> + max_payload_size = loopback_mtu_size - ipv4_udp_header_size;
> + sendto(syslog_sink_fd, ds_cstr_ro(syslog_message),
> + MIN(syslog_message->length, max_payload_size), 0,
> + (struct sockaddr *)(&ipaddr), sizeof ipaddr);
> +}
> +
> /* Writes 'message' to the log at the given 'level' and as coming from the
> * given 'module'.
> *
> @@ -837,6 +933,7 @@ vlog_valist(const struct vlog_module *module, enum
> vlog_level level,
> }
>
> if (log_to_syslog) {
> + int target_udp_port = 0;
> int syslog_level = syslog_levels[level];
> char *save_ptr = NULL;
> char *line;
> @@ -846,6 +943,12 @@ vlog_valist(const struct vlog_module *module, enum
> vlog_level level,
> line = strtok_r(NULL, "\n", &save_ptr)) {
> syslog(syslog_level, "%s", line);
> }
> +
> + if (udp_syslog_export_enabled(&target_udp_port)) {
> + format_log_message(
> + module, level, VLF_RFC5424, message, args, &s);
> + export_to_udp_syslog_target(&s, target_udp_port);
> + }
> }
>
> if (log_to_file) {
> @@ -1013,9 +1116,12 @@ void
> vlog_usage(void)
> {
> printf("\nLogging options:\n"
> - " -v, --verbose=[SPEC] set logging levels\n"
> - " -v, --verbose set maximum verbosity level\n"
> - " --log-file[=FILE] enable logging to specified FILE\n"
> - " (default: %s/%s.log)\n",
> + " -v, --verbose=[SPEC] set logging levels\n"
> + " -v, --verbose set maximum verbosity level\n"
> + " --log-file[=FILE] enable logging to specified FILE\n"
> + " (default: %s/%s.log)\n"
> + " --syslog-target=[UDP_PORT] export syslog messages to the \n"
> + " local UDP_PORT in addition to \n"
> + " the system syslog\n",
> ovs_logdir(), program_name);
> }
> diff --git a/lib/vlog.h b/lib/vlog.h
> index d7d63bf..05c65a8 100644
> --- a/lib/vlog.h
> +++ b/lib/vlog.h
> @@ -62,9 +62,11 @@ enum vlog_level vlog_get_level_val(const char *name);
>
> /* Facilities that we can log to. */
> #define VLOG_FACILITIES \
> - VLOG_FACILITY(SYSLOG, "ovs|%05N|%c%T|%p|%m") \
> + VLOG_FACILITY(SYSLOG, "ovs|%05N|%c%T|%p|%m") \
> VLOG_FACILITY(CONSOLE, "%D{%Y-%m-%dT%H:%M:%SZ}|%05N|%c%T|%p|%m") \
> - VLOG_FACILITY(FILE, "%D{%Y-%m-%dT%H:%M:%S.###Z}|%05N|%c%T|%p|%m")
> + VLOG_FACILITY(FILE, "%D{%Y-%m-%dT%H:%M:%S.###Z}|%05N|%c%T|%p|%m") \
> + VLOG_FACILITY(RFC5424, \
> + "<%B>1 %D{%Y-%m-%dT%H:%M:%S.###Z} %E %A %P %c - \xEF\xBB\xBF%m")
> enum vlog_facility {
> #define VLOG_FACILITY(NAME, PATTERN) VLF_##NAME,
> VLOG_FACILITIES
> @@ -139,6 +141,9 @@ void vlog_set_pattern(enum vlog_facility, const char
> *pattern);
> int vlog_set_log_file(const char *file_name);
> int vlog_reopen_log_file(void);
>
> +/* Configure syslog target. */
> +void vlog_set_syslog_target(const char *target);
> +
> /* Initialization. */
> void vlog_init(void);
> void vlog_enable_async(void);
> @@ -213,17 +218,26 @@ void vlog_rate_limit(const struct vlog_module *, enum
> vlog_level,
> #define VLOG_DBG_ONCE(...) VLOG_ONCE(VLL_DBG, __VA_ARGS__)
>
> /* Command line processing. */
> -#define VLOG_OPTION_ENUMS OPT_LOG_FILE
> -#define VLOG_LONG_OPTIONS \
> - {"verbose", optional_argument, NULL, 'v'}, \
> - {"log-file", optional_argument, NULL, OPT_LOG_FILE}
> +#define VLOG_OPTION_ENUMS \
> + OPT_LOG_FILE, \
> + OPT_SYSLOG_TARGET
> +
> +#define VLOG_LONG_OPTIONS \
> + {"verbose", optional_argument, NULL, 'v'}, \
> + {"log-file", optional_argument, NULL, OPT_LOG_FILE}, \
> + {"syslog-target", optional_argument, NULL, OPT_SYSLOG_TARGET}
> +
> #define VLOG_OPTION_HANDLERS \
> case 'v': \
> vlog_set_verbosity(optarg); \
> break; \
> case OPT_LOG_FILE: \
> vlog_set_log_file(optarg); \
> + break; \
> + case OPT_SYSLOG_TARGET: \
> + vlog_set_syslog_target(optarg); \
> break;
> +
> void vlog_usage(void);
>
> /* Implementation details. */
> diff --git a/lib/vlog.man b/lib/vlog.man
> index 39edaee..1410394 100644
> --- a/lib/vlog.man
> +++ b/lib/vlog.man
> @@ -60,3 +60,7 @@ Sets the log pattern for \fIfacility\fR to \fIpattern\fR.
> Refer to
> Enables logging to a file. If \fIfile\fR is specified, then it is
> used as the exact name for the log file. The default log file name
> used if \fIfile\fR is omitted is \fB@LOGDIR@/\*(PN.log\fR.
> +.TP
> +\fB\-\-syslog\-target\fR[\fB=\fIudp_port\fR]
> +export syslog messages to the local \fIudp_port\fR in addition to the
> +system syslog.
> diff --git a/utilities/ovs-appctl.8.in b/utilities/ovs-appctl.8.in
> index 1cf888d..e381b2b 100644
> --- a/utilities/ovs-appctl.8.in
> +++ b/utilities/ovs-appctl.8.in
> @@ -148,6 +148,9 @@ expanded as follows:
> .IP \fB%A\fR
> The name of the application logging the message, e.g. \fBovs\-vswitchd\fR.
> .
> +.IP \fB%B\fR
> +The RFC5424 syslog PRI of the message.
> +.
> .IP \fB%c\fR
> The name of the module (as shown by \fBovs\-appctl \-\-list\fR) logging
> the message.
> @@ -173,6 +176,9 @@ takes the same format as the \fItemplate\fR argument to
> \fBstrftime\fR(3). Supports the same extension for sub-second
> resolution as \fB%d{\fR...\fB}\fR.
> .
> +.IP \fB%E\fR
> +The hostname of the node running the application.
> +.
> .IP \fB%m\fR
> The message being logged.
> .
> --
> 1.8.5.rc1.17.g0ecd94d
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
