News for hackers. Stuff that matters.
Re:Systemd is bad parsing by flohofwoe on Saturday October 27, 2018 @08:49AM (#18315492)
I agree with most of the critique, but I don't quite get the part about the use of assert.

The code in question quite clearly doesn't use C's standard assert macro, but a custom macro called assert_return (which I guess checks for a condition, and returns from the function instead of aborting the program).

So basically:

    if (!condition) {
        return err;
    }
How else would one check for invalid input to the function?
Re:Systemd is bad parsing by bjourne on Saturday October 27, 2018 @09:56AM (#18315694)
I don't know the details of DHCP, but there is a difference between values supplied by the programmer and those supplied by the user. According to the author, checking if a pointer is not-null is checking a programmer error (EINVAL) so it makes sense to use asserts. But the optval string in the example is supplied by the user so it is insane to use asserts to check for null-termination.

Asserts should only be used to signal "this code is broken" but that is not how they are used here.

A long while ago the GTK+ library used to suffer from a similar affliction. Mainstay GTK+ programs such as GEdit and Totem would print lots of warnings on the console. Since the GTK+ developers used the same facility (g_warning) for reporting both coding errors and for user errors like "file not found," debugging code was hard. Was the warning "supposed to be there" or was it a bug? GTK+ programs are much cleaner these days even if some programs still fails to make the distinction between user and programmer errors.

Re:Systemd is bad parsing by viraptor on Saturday October 27, 2018 @09:12AM (#18315552)
I may have a different understanding than the author, but for me the difference in the name means a lot. The fragment you wrote is clear and simple. But if I see assert_..., then I immediately have questions like: Where's the message printed and does it have enough context for debugging at that point? Is this affected by NDEBUG? Will it run in release builds? Does assert_se() kill the whole application?

Assert already has a meaning and using it in this context is not great.

Re:Systemd is bad parsing by mrami on Saturday October 27, 2018 @10:01AM (#18315716)
Yes, exactly. Coming from C++/Java, I immediately though, "wait, this is turned off in production?"

cf. http://www.cplusplus.com/reference/cassert/assert/

Re:Systemd is bad parsing by XorNot on Saturday October 27, 2018 @09:13AM (#18315553)
I can see the argument to be made about overloading the word "assert" in the language though.

i.e. "assert" meaning "check something in this code is algorithmically correct"

Whereas when you're checking user input, use a very different term - i.e. validate or something.

So a fix would be to rename the macro to be obvious about it's usage - "validate_return" or "valid_or_return" or something.

To be fair I can see the problem because I do tend to write code comments saying "assert everything makes sense" when checking user input sometimes and "assert" itself doesn't properly convey the outcome if it fails - i.e. "abort_if" or "crash_if" would be a much more descriptive term.

Re:Systemd is bad parsing by tetha on Saturday October 27, 2018 @02:49PM (#18317122)
In my opinion diluting existing, well-established terms is extremely dangerous and harmful for a project. If you're diluting established terms - like assert - you're kinda forcing readers to stop assuming anything about your names.

If assert isn't assert, and if return values are also abused, why can I assume get is get, sort is sort and so on? Sort might check inputs, get might delete the customer account, who knows at this point. And that makes anything but a tiny code base pretty much unmanageable, or at least very very hard to learn.

Re:Systemd is bad parsing by edoceo on Saturday October 27, 2018 @10:44AM (#18315881)
I used assert_* for code bugs and then verify_* for the user checking ones, my projects would have a verify.h,c file that was pretty short/sweet/easy for the next dev to grok.

At one point and older coder who I looked up to even complemented and adopted this pattern. 1998 was a good year.

Re:Systemd is bad parsing by Flow on Saturday October 27, 2018 @10:01AM (#18315717)
Words matter.

I sometimes use a bunch of Ensure*() methods in C# if I need to validate input data. They throw exceptions if condition fails. If I only do this once I don't create those methods.

One drawback is the stacktrace has these Ensure names in it.

Re:Systemd is bad parsing by corndoge on Saturday October 27, 2018 @11:27AM (#18316145)
Probably returns a value if the assert works, and aborts otherwise.

I would go so far as to say that using asserts in input validation is a good thing, in very limited quantities. I use them in places where the input should have been validated so long ago, so many functions back, that if something isn't as I expect here, I seriously fucked up somewhere else and the whole system has a good likelihood of being compromised by something unrelated to the condition I'm asserting. In that scenario I'd rather log and abort.

Re:Systemd is bad parsing by int_19h on Saturday October 27, 2018 @03:49PM (#18317375)
In this case, though, you're not using asserts to validate. You're using asserts to capture the post-validation guarantees in your code. Basically, you assert that your validation code did the right thing and rejected invalid input through appropriate mechanisms - not that the input was valid.
Re:Systemd is bad parsing by LukeShu on Saturday October 27, 2018 @07:38PM (#18318310)
That's what systemd does. For instance, the article uses a couple of asserts in dhcp6_option_parse_domainname(). By the time that function is getting called, the values should have already been validated, and the asserts (which are at the very top of the function) are asserting that the validation code did the right thing.
Re:Systemd is bad parsing by int_19h on Sunday October 28, 2018 @03:32PM (#18322541)
Then why do those "asserts" have return error codes associated with them? A failing assert shouldn't use normal error reporting mechanisms, and it most certainly shouldn't return a specific error code implying that assertion failures can (and should) be handled.
Re:Systemd is bad parsing by LukeShu on Sunday October 28, 2018 @03:40PM (#18322622)
For the same reason read(3) returns EBADF if you pass it a bad file descriptor. Why doesn't the libc wrapper around the syscall just abort() if I give it a bad file descriptor?

In systemd-networkd, the "asserts" assert the pre-conditions of a successful call to that function (in dhcp6-option.c). It's up to the caller (in sd-dhcp6-lease.c) to validate the users input, and ensure that it's passing valid arguments to the functions it's calling.

Building software in layers, and making use of "internal" libraries is a valid software engineering strategy for managing complexity.

Re:Systemd is bad parsing by int_19h on Monday October 29, 2018 @12:03AM (#18324969)
> For the same reason read(3) returns EBADF if you pass it a bad file descriptor. Why doesn't the libc wrapper around the syscall just abort() if I give it a bad file descriptor?

Because, unfortunately, POSIX requires it to do that. But that's a bad decision on the part of POSIX, and we shouldn't perpetuate that when we have the choice. Assertions that can be "handled" are not assertions - they're errors. By returning an error code, you're saying "it's okay to pass bad things here without checking, and check the error code afterwards". If it's not actually okay, because validation is not reliable, then you're just enabling writing bad, unreliable code that shouldn't even be possible to write.

The "bad descriptor" check in read() is a good example. You can never rely on EBADF unless you fully control every line of code that ever runs in your process, because a descriptor might be accidentally valid - it might be a descriptor that was closed, but then got reused for something else (and POSIX explicitly requires reuse of file descriptors). So checking for EBADF to e.g. detect closed descriptors and do something about that will mostly work, but it will sporadically break by doing something on the wrong descriptor. Therefore, it would be strictly better if read() just called abort(), and thereby forced the caller to use some other mechanism to keep track of what's valid and what isn't - the one that the caller can implement reliably, if it can.

Re:Systemd is bad parsing by FeepingCreature on Saturday October 27, 2018 @01:07PM (#18316642)
In D, we call the two functions "assert" and "enforce". The point is that assert is the wrong term; to say "assert(condition)" says "I assert that condition should hold here". If there's an input event that causes condition to be false then your assert is not just false but wrong, since you assert something that is not necessarily true.