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 | 115 ++++++++++++++++++++++++++++++++++++++++++++--
lib/vlog.h | 26 ++++++++---
lib/vlog.man | 4 ++
utilities/ovs-appctl.8.in | 6 +++
5 files changed, 142 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..8cd57c0 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,18 @@ 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 +616,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 +635,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 +649,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 +751,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 +790,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 +806,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 +864,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 +934,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 +944,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 +1117,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
