Page MenuHomeFreeBSD

Add support for parsing UCL config files to newsyslog
Needs ReviewPublic

Authored by allanjude on Jan 17 2015, 1:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 24 2024, 3:57 PM
Unknown Object (File)
Feb 22 2024, 11:13 PM
Unknown Object (File)
Feb 19 2024, 8:16 PM
Unknown Object (File)
Jan 24 2024, 3:58 PM
Unknown Object (File)
Jan 23 2024, 12:09 AM
Unknown Object (File)
Jan 9 2024, 10:23 AM
Unknown Object (File)
Dec 10 2023, 4:49 AM
Unknown Object (File)
Dec 4 2023, 5:02 AM
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Lint Coverage
Unit
No 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
It also allows for 'versioning' of the config schema, so we can easily keep backwards compatibility if we change the config syntax again in the future.

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
or something similar

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
1088

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

1485

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

1689

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
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).

1087

Check the return value of fgets(3) before using line.

1093

style(9): The body of the if() statement goes on a line by itself.

1094–1097

Floating point equality is somewhat fragile. Use integers for this.

1502

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.

1508

extra space (style)

1514

extra space

1520–1522

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.

1656

style(9): return (true);

1664–1665

Ah, I see, you force octal here. Ok.

1699–1700

these should be on the same line (style(9))

Update with feedback from cem

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?)

Changes between 3218 and 3220 LGTM.

usr.sbin/newsyslog/newsyslog.c
1692–1693

These on the same line too

1734

extra space

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.

1088–1092

You can drop the lineptr variable entirely. You would use strtod like:

uclver = strtod(&line[5], NULL);

1487–1498

I think you can drop the (conf_helper_func) casts if you change the function typedef from void * to struct conf_entry *.

1551

The usual pattern for this is:

if ((working->flags & CE_NOSIGNAL) != 0) {

1606–1607

You can drop result entirely.

target->uid = ucl_object_toint(obj);

1612

Use atol or strtol. uid_t is unsigned and yes, somebody has >2 billion uids (or at least uses the high numbers).

1613

target->uid = atol(val); or similar

1624–1625

target->uid = pwd->pw_uid;

1641

extra parens, you don't need them around function calls

1649

Probably atol here too.

1661

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.

1675

(same re: sscanf(...) != 1.)

1712

No 'n' needed for xz, I think?

1793–1794

same line

1796–1797

also same line

1800–1801

same

1805–1811

braces can be dropped here.

1825–1826

same

1886

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)

3143

extra space

3156

If you want to reduce indentation, you can do:

if (strcasecmp(...) != 0)
  continue;

And then the rest of the loop body can be unindented.

3158–3160

Can drop NULL check entirely. Just don't put a NULL callback in your table :-).

3164

if (!found && ...)

3170

(ret)

Update with more feedback from cem

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.

1088–1092

dropped

1487–1498
1502

I moved the rewind(cf) that was done only in the case of a non-ucl config earlier, so it should resolve this

1551

Not my code, but since it is inside my function, fixing it

1606–1607

ok

1612

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.

1641

those were existing, but removed

1712

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.

3158–3160

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.

LGTM.

usr.sbin/newsyslog/newsyslog.c
1712

Ah, I got bzip/bzip2 but hadn't seen xzip before. Seems reasonable to me.

3158–3160

Ok.

3164

Typo — I think you want !found?

usr.sbin/newsyslog/newsyslog.c
1712

This question made me think about it more, and improve it further
use strncasecmp(val, ..., 2) in all cases:

so gzip accepts: gz gzip
bzip2: bz bz2 bzip bzip2
xz: xz xzip yes
none: no none

(added yes to xz since we accept no)

3164

good catch, thank you

Further expand the accepted input for the compression parameter

LGTM.

usr.sbin/newsyslog/newsyslog.c
1704

Maybe just strcasecmp() for "yes"?

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.

In D1548#23, @mat wrote:

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.

Correct, I've not added that yet. I'll get that done shortly.

overall reasonable, just some quick comments.

usr.sbin/newsyslog/newsyslog.c
1083

style(9) prefers not having init inline

1095

theoretical overflow not handled, but is likely fine

1517

TODO comment :)

1540

if (!success) {} ?

1782

if ul == 0, check errno?

Address Feedback