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

Henry Mai henrymai at nicira.com
Wed Dec 4 05:59:19 UTC 2013


Sorry, found another violation where I had an extra newline before a function.

On Tue, Dec 3, 2013 at 8:30 PM, Henry Mai <henrymai at nicira.com> wrote:
> Another patch to address style issues
>
> On Tue, Dec 3, 2013 at 3:19 PM, Henry Mai <henrymai at nicira.com> wrote:
>> Here's an updated patch.
>>
>> On Mon, Dec 2, 2013 at 2:06 PM, Ben Pfaff <blp at 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 <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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-vlog-add-local-udp-syslog-export-target-option.patch
Type: text/x-patch
Size: 12424 bytes
Desc: not available
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20131203/19cae1f7/attachment-0005.bin>


More information about the dev mailing list