Page MenuHomeFreeBSD

daemon: add long_opts
ClosedPublic

Authored by ihor_antonovs.family on Jan 28 2023, 5:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Oct 18, 4:04 AM
Unknown Object (File)
Thu, Oct 17, 3:54 PM
Unknown Object (File)
Thu, Oct 17, 3:39 PM
Unknown Object (File)
Thu, Oct 17, 3:39 PM
Unknown Object (File)
Wed, Oct 16, 10:30 AM
Unknown Object (File)
Wed, Oct 16, 4:50 AM
Unknown Object (File)
Tue, Oct 15, 5:46 PM
Unknown Object (File)
Mon, Oct 14, 7:15 PM

Details

Summary

Long options improve readability of scripts, makes code comprehension easier.
This patch adds long options while preserving existing CLI interface.
Also --help/-h option is added

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ihor_antonovs.family retitled this revision from [WIP] daemon: add long_opts to daemon: add long_opts.
pauamma_gundo.com added inline comments.
usr.sbin/daemon/daemon.8
82–83

Should the defaults be given both here and in the individual flag descriptions?

89–91

I'm not sure whether here and elsewhere in DESCRIPTION where options (other than the one being described, ie -o --output-file here) are mentioned, it's better to use the long form or the short form as diff(1) and others do. Opinions welcome.

158

s/syslog tag/syslog-tag/

usr.sbin/daemon/daemon.8
89–91

IMHO long form improves readability everywhere, I don't have to look up what -H means every time. Short options are difficult to remember unless you work with this tool every day.

But that being said I am happy to revert this if other reviewers feel the same.

Meta: please generate phab diffs with full context included (-U999999, just really high); it makes reviewing a bit easier in the average case where additional context in the file might affect the review process (e.g., checking for consistency)

usr.sbin/daemon/daemon.8
60

Prevailing style is to insert a comma between the short option and long option in these listings, e.g.,

.It Fl c , Fl -change-dir
usr.sbin/daemon/daemon.c
86

Might as well const this

134

I think we can drop this line, it's generally assumed that one will seek out the manpage for details.

ihor_antonovs.family marked 2 inline comments as done.

missing closing brace

ihor_antonovs.family marked an inline comment as done and an inline comment as not done.Feb 2 2023, 6:36 AM
ihor_antonovs.family marked an inline comment as done.

@kevans: fixed all comments, added diff with -U9999999

Manual page changes LGTM. Can't attest to source code or consistency between them.

This revision is now accepted and ready to land.Feb 2 2023, 1:31 PM

how about --help?

usr.sbin/daemon/daemon.c
133

why not add a --help while you're at it?

this way, drop the exit(1) from usage(), and exit as appropriate from main().

usr.sbin/daemon/daemon.c
133

I was tempted to do this, but I was not sure if reviewers will like it. Not everybody likes the scope creep. I was planning to do this in a separate revision, but I will gladly add it to this diff!

This revision now requires review to proceed.Feb 3 2023, 8:57 PM
usr.sbin/daemon/daemon.c
110

how we could pass on an option here if we're printing onto stderr or stdout, because often you'd want to foo --help | grep topic

but i'd leave this to more experienced people

usr.sbin/daemon/daemon.c
110

I like the idea and this is easy to do, but I will keep this feature for the next diff, I want to keep this change scoped

allanjude added a subscriber: allanjude.
allanjude added inline comments.
usr.sbin/daemon/daemon.c
85

l: is specified twice

233

without the braces around this if, this exit(1) runs unconditionally

This revision now requires changes to proceed.Feb 4 2023, 5:48 PM
usr.sbin/daemon/daemon.c
222

usage(0) would be better, and adjust usage to take a parameter and exit that value. Avoids errors like no {} from creeping in.

usr.sbin/daemon/daemon.c
222

and could double for the other requested change, 'if usage() is not an error, write to stdout instead of stderr'

usr.sbin/daemon/daemon.c
85

It is not me, it was like that before, I promise :)

Will fix.

222

ack, will do

233

ah, my bad , that was late change after adding --help...

usr.sbin/daemon/daemon.c
222

I'm not aware of another program does that in the tree, so I'd suggest that we don't do that here.

add __builtin_unreachable(); for --help case
remove unnecessary usage() prototype

allanjude accepted this revision.
This revision is now accepted and ready to land.Feb 5 2023, 5:57 PM
This revision is now accepted and ready to land.Feb 6 2023, 8:32 PM
This revision was automatically updated to reflect the committed changes.