Looks fine to me. Thanks.
On Thu, Dec 5, 2013 at 5:02 PM, Ben Pfaff <b...@nicira.com> wrote: > 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
