Missing man page updates
Requires newer version of UCL than is included in base (remove hack in Makefile once base is updated, in meantime use port)
Requires much review, I am a very novice C programmer
Details
- Reviewers
- None
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint OK - Unit
No Unit Test Coverage
Event Timeline
etc/newsyslog.ucl | ||
---|---|---|
1 | This 'sentinel' lets newsyslog know that this file is in FreeBSD UCL version 0.1. If it does not see this at the top of the file, it treats it as a legacy .conf file in the original format. This will allow the new format to keep the original filename, newsyslog.conf | |
3 | This is a comment, of the original line from newsyslog.conf | |
26 | A new format for the time specification would likely be very welcome | |
203 | Working with Vsevolod Stakhov (cebka@) to come up with a way to do opaque strip splitting (https://github.com/vstakhov/libucl/issues/36) so this could be: flags = binary, nosignal | |
usr.sbin/newsyslog/Makefile | ||
9 | Dirty hack until contrib/libucl is updated. Not in a rush for this as there are some more features to be added to libucl that would be useful for this project. | |
usr.sbin/newsyslog/newsyslog.c | ||
1086 | taste the first line of the file. If it isn't what we want, rewind() and continue as before Otherwise, pass it off to the new config parser | |
1488 | This is the map that uses the callbacks. It ended up different than I originally pictures, but I think it is still nicer than a giant strcmp() if ladder | |
1692 | This is mostly just verbatim from the original code. It can likely be improved, but I wanted to try to avoid introducing too many new bugs at once |
Not a super thorough review, just passed over briefly. I'm not an expert on UCL nor newsyslog.
etc/newsyslog.ucl | ||
---|---|---|
5 | Note to other reviewers: UCL doesn't implicitly understand 'mode' is octal, we just parse this field specially later in the patch. | |
usr.sbin/newsyslog/newsyslog.c | ||
1082 | Using double for this is odd. Usually we just multiply by a power of 10 (or 0x10). | |
1085 | Check the return value of fgets(3) before using line. | |
1091 | style(9): The body of the if() statement goes on a line by itself. | |
1092–1095 | Floating point equality is somewhat fragile. Use integers for this. | |
1505 | You probably want to lseek(2) the fd back to the beginning — fgets(3) and other stdio functions may buffer large chunks of the file and the fd may be well past the beginning of the UCL stuff. | |
1511 | extra space (style) | |
1517 | extra space | |
1523–1525 | There's lots of this, so I'm not going to mark it every time. But, in style(9) it is suggested that if/while/for braces be omitted when they are not required (with exceptions for complicated bodies). printf is not complicated. | |
1659 | style(9): return (true); | |
1667–1668 | Ah, I see, you force octal here. Ok. | |
1702–1703 | these should be on the same line (style(9)) |
usr.sbin/newsyslog/newsyslog.c | ||
---|---|---|
1082 | So make 'uclver' an int, and multiply the result of strtod(3) by 10? |
usr.sbin/newsyslog/newsyslog.c | ||
---|---|---|
1082 | That'd work. You could also just use an integer version (e.g. 1) and strtol or whatever. (This is just versioning the config schema, right?) |
Looks pretty good!
usr.sbin/newsyslog/newsyslog.c | ||
---|---|---|
212 | Don't need the backslash. typedef isn't part of the preprocessor. | |
213 | It looks like the 3rd parameter here should be struct conf_entry * based on later functions. | |
1086–1090 | You can drop the lineptr variable entirely. You would use strtod like: uclver = strtod(&line[5], NULL); | |
1490–1501 | I think you can drop the (conf_helper_func) casts if you change the function typedef from void * to struct conf_entry *. | |
1554 | The usual pattern for this is: if ((working->flags & CE_NOSIGNAL) != 0) { | |
1609–1610 | You can drop result entirely. target->uid = ucl_object_toint(obj); | |
1615 | Use atol or strtol. uid_t is unsigned and yes, somebody has >2 billion uids (or at least uses the high numbers). | |
1616 | target->uid = atol(val); or similar | |
1627–1628 | target->uid = pwd->pw_uid; | |
1644 | extra parens, you don't need them around function calls | |
1652 | Probably atol here too. | |
1664 | Probably: if (sscanf(...) != 1) {. sscanf may return EOF (-1) as well as (0) depending on what went wrong. Anything other than 1 conversion is failure for us. | |
1678 | (same re: sscanf(...) != 1.) | |
1715 | No 'n' needed for xz, I think? | |
1796–1797 | same line | |
1799–1800 | also same line | |
1803–1804 | same | |
1808–1814 | braces can be dropped here. | |
1828–1829 | same | |
1889 | You can drop the comment, the pattern is well understood. Alternatively, you can include sys/cdefs.h (if it isn't already) and use __unused like: conf_set_unknown(..., struct conf_entry *target __unused) | |
3129 | extra space | |
3142 | If you want to reduce indentation, you can do: if (strcasecmp(...) != 0) continue; And then the rest of the loop body can be unindented. | |
3144–3146 | Can drop NULL check entirely. Just don't put a NULL callback in your table :-). | |
3150 | if (!found && ...) | |
3156 | (ret) |
usr.sbin/newsyslog/newsyslog.c | ||
---|---|---|
213 | I originally had this code in libucl, so it was generalized to work in places other than just newsyslog. I just dumped it into newsyslog for now. | |
1086–1090 | dropped | |
1490–1501 | see response to https://reviews.freebsd.org/D1548#inline-9104 | |
1505 | I moved the rewind(cf) that was done only in the case of a non-ucl config earlier, so it should resolve this | |
1554 | Not my code, but since it is inside my function, fixing it | |
1609–1610 | ok | |
1615 | this case doesn't need to be here, it is left over. If the user entered a number, it will by type UCL_INT, unless they put a decimal in it or something silly, in which case they deserve an error anyway. | |
1644 | those were existing, but removed | |
1715 | It is done so it allows the user to do 'xzip' as well. same reason bzip is cut to 4 chars, so bzip or bzip2 both are accepted. | |
3144–3146 | This is there on purpose. We have a NULL callback for the 'file' key, because it is handled elsewhere, and we don't want that key to trip the 'default' callback. See line #1486 This should be documented better, my goal is to make something generic to speed up the process of converting other utilities to UCL. |
Mmm, newsyslog.conf has:
<include> /etc/newsyslog.conf.d/* <include> /usr/local/etc/newsyslog.conf.d/*
This doesn't seem to have been included here.