[ovs-dev] [PATCH] vlog: add local udp syslog export target option

[Ben Pfaff]

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 <henrymai at 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.