[ovs-dev] [PATCH] vlog: add local udp syslog export target option
Henry Mai
henrymai at nicira.com
Wed Dec 4 04:30:11 UTC 2013
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: 12426 bytes
Desc: not available
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20131203/35e77db5/attachment-0005.bin>
More information about the dev
mailing list