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
Details
- Reviewers
- None
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository
- Lint
Lint OK - Unit
No Unit Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| etc/newsyslog.ucl | ||
|---|---|---|
| 2 | 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 | |
| 4 | This is a comment, of the original line from newsyslog.conf | |
| 27 | A new format for the time specification would likely be very welcome | |
| 204 | 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 | ||
| 10 | 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 | ||
| 1087 | 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 | |
| 1489 | 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 | |
| 1693 | 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 | |
Comment Actions
Not a super thorough review, just passed over briefly. I'm not an expert on UCL nor newsyslog.
| etc/newsyslog.ucl | ||
|---|---|---|
| 6 | 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 | ||
| 1083 | Using double for this is odd. Usually we just multiply by a power of 10 (or 0x10). | |
| 1086 | Check the return value of fgets(3) before using line. | |
| 1092 | style(9): The body of the if() statement goes on a line by itself. | |
| 1093–1096 | Floating point equality is somewhat fragile. Use integers for this. | |
| 1506 | 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. | |
| 1512 | extra space (style) | |
| 1518 | extra space | |
| 1524–1526 | 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. | |
| 1660 | style(9): return (true); | |
| 1668–1669 | Ah, I see, you force octal here. Ok. | |
| 1703–1704 | these should be on the same line (style(9)) | |
| usr.sbin/newsyslog/newsyslog.c | ||
|---|---|---|
| 1083 | So make 'uclver' an int, and multiply the result of strtod(3) by 10? | |
| usr.sbin/newsyslog/newsyslog.c | ||
|---|---|---|
| 1083 | 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?) | |
Comment Actions
Looks pretty good!
| usr.sbin/newsyslog/newsyslog.c | ||
|---|---|---|
| 213 | Don't need the backslash. typedef isn't part of the preprocessor. | |
| 214 | It looks like the 3rd parameter here should be struct conf_entry * based on later functions. | |
| 1087–1091 | You can drop the lineptr variable entirely. You would use strtod like: uclver = strtod(&line[5], NULL); | |
| 1491–1502 | I think you can drop the (conf_helper_func) casts if you change the function typedef from void * to struct conf_entry *. | |
| 1555 | The usual pattern for this is: if ((working->flags & CE_NOSIGNAL) != 0) { | |
| 1610–1611 | You can drop result entirely. target->uid = ucl_object_toint(obj); | |
| 1616 | Use atol or strtol. uid_t is unsigned and yes, somebody has >2 billion uids (or at least uses the high numbers). | |
| 1617 | target->uid = atol(val); or similar | |
| 1628–1629 | target->uid = pwd->pw_uid; | |
| 1645 | extra parens, you don't need them around function calls | |
| 1653 | Probably atol here too. | |
| 1665 | 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. | |
| 1679 | (same re: sscanf(...) != 1.) | |
| 1716 | No 'n' needed for xz, I think? | |
| 1797–1798 | same line | |
| 1800–1801 | also same line | |
| 1804–1805 | same | |
| 1809–1815 | braces can be dropped here. | |
| 1829–1830 | same | |
| 1890 | 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) | |
| 3130 | extra space | |
| 3143 | If you want to reduce indentation, you can do: if (strcasecmp(...) != 0) continue; And then the rest of the loop body can be unindented. | |
| 3145–3147 | Can drop NULL check entirely. Just don't put a NULL callback in your table :-). | |
| 3151 | if (!found && ...) | |
| 3157 | (ret) | |
| usr.sbin/newsyslog/newsyslog.c | ||
|---|---|---|
| 214 | 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. | |
| 1087–1091 | dropped | |
| 1491–1502 | see response to https://reviews.freebsd.org/D1548#inline-9104 | |
| 1506 | I moved the rewind(cf) that was done only in the case of a non-ucl config earlier, so it should resolve this | |
| 1555 | Not my code, but since it is inside my function, fixing it | |
| 1610–1611 | ok | |
| 1616 | 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. | |
| 1645 | those were existing, but removed | |
| 1716 | 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. | |
| 3145–3147 | 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. | |
Comment Actions
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.