Page MenuHomeFreeBSD

daemon(8): Add support for logging via syslog
Needs RevisionPublic

Authored by feld on Sep 19 2016, 3:36 PM.

Details

Reviewers
mjg
Summary

A developer emailed me this code after seeing it listed on the Junior Jobs page.
Their diff was before the -t for title was added, so I adjusted it to be
-T for the syslog tag.

If deemed acceptable I will write the man page update as well.

Original author is John Grell <grell64@gmail.com>

Diff Detail

Event Timeline

feld updated this revision to Diff 20487.Sep 19 2016, 3:36 PM
feld retitled this revision from to daemon(8): Add support for logging via syslog.
feld updated this object.
feld edited the test plan for this revision. (Show Details)
feld updated this object.Sep 19 2016, 3:43 PM
mjg requested changes to this revision.Sep 21 2016, 12:27 PM
mjg added a reviewer: mjg.
mjg added a subscriber: mjg.

The idea seems acceptable and the approach of offloading everything to logger(1) makes sense on the first sight.

However, the patch in its current form needs significant changes.

usr.sbin/daemon/daemon.c
48

Why is this line here and not with the rest of includes?

68

Newly added letters unsort the list. The new list is missing 'T:'.

109

What's up with this line?

214

What's up with this comment? In general, if you look around, there are no comments of the sort in the file.

216

The style used in the file (and most of the codebase) is "if (condition) {". "{" on a separate lines is inconsistent with your own further uses later.

221

All variables are supposed to be declared at the beginning of the function.

225

The first line used "sizeof tagstring", but the second hardcodes 199. Doing snprintf followed by strncat is very odd. This should just snprintf to the target buffer. The argument should be quoted.

style(9) requires parenthesis around the argument, i.e. "sizeof(tagstring)".

232

popen is a bad idea here. This program execve's out of the code dong the original popen making it impossible to do a proper cleanup with pclose. While likely harmless, it does look weird.

The real problem is the fact that the string goes through the shell which introduces unnecessary complications due to shell parsing.

Instead, the code should fork + exec on its own.

233

Nothing informative is printed should popen fail.

242

This execvp plays no role. If it fails, the code drops down to line 247 which contains an idential execvp.

326

This line now significantly exceeds 80 characters mark.

This revision now requires changes to proceed.Sep 21 2016, 12:27 PM
mjg added a comment.Sep 2 2019, 1:26 PM

Any news on this?