Replace FOREACH_WORD_QUOTED with a loop using unquote_first_word in config_parse_exec() #44
Conversation
src/core/load-fragment.c
Outdated
There was a problem hiding this comment.
Do I need a log_*_errno here? Other places in this function will log, and I also have return log_oom() elsewhere... What would be an appropriate log for places where I'm returning -EINVAL or similar errors from unquote_first_word?
There was a problem hiding this comment.
Yes, I think that would help users to spot problems in their unit files. Just use log_syntax(), as that will already include the current line and file that is parsed, and augment that with whatever caused the error (in this case, 'p').
There was a problem hiding this comment.
Done.
Using log_syntax() for errors from unquote_first_word() and using log_oom() if some strv operation fails (this is a pattern I've also seen elsewhere.)
filbranden
force-pushed
the
unquote_first_word1
branch
from
bab7886 to
c34e9e6
Compare
|
I updated the commit to:
I think this one is ready to go. Would you please take another look? Let me know if there's anything that needs to be addressed (or feel free to take over, download the pull request and do small changes before you push it.) Cheers, |
|
cc @arvidjaar who commented on this patchset before. |
|
Thanks for your comments! I'd appreciate it if you could answer my questions above. I'll try to send you an updated patchset over the weekend. Cheers, |
filbranden
force-pushed
the
unquote_first_word1
branch
from
c34e9e6 to
a475d28
Compare
|
Updated according to the comments and added a lot more tests. Also split the new tests into a separate prequel commit, so that the behavior changes would be very explicit in the changes to the test results. Tested it to make sure Please take another look. Cheers, |
|
Unfortunately it doesn't apply anymore! Sorry for the long delay in reviewing. Any chance you can rebase this? Will merge then! |
|
I'll rebase and retest today... Or if you prefer to delay this until after v221 release just let me know and I'll postpone it. Ok to reuse the same PR? I guess as there are no line comments really then it should be fine... If I don't hear from you again about postponing for after v221, then expect rebased and retested later today from me. Cheers, |
|
sounds good to merge this before the release. it has tests and stuff after all. reusing the PR is fine, though usually we rty to avoid it... |
|
I don't want to make a big fuss, but we've had way more than just one regression due to parser changes in the past, including really bad ones which crashed pid 1. Usually stuff like that isn't the best thing to land two days before making a new release.. |
|
@martinpitt Yeah I kind of agree with you... I'll leave the final decision to @poettering though, so I think I'll rebase and push and then this can be merged either before or after v221 (I don't expect it to get out of sync between now and then.) If you prefer to only merge parts of it now (such as the extra unit tests) I'd be happy to move only those commits that do not change behavior to another PR so that it can be merged now (or feel free to just check out this PR branch and cherry-pick them yourself.) Cheers! |
|
There are two days left, and it's the days where people probably test the most, hence please go ahead with the full patch set. |
The new flag UNQUOTE_UNESCAPE_RELAX preserves unrecognized escape sequences verbatim in unquote_first_word, either when it's a trailing backslash (similar to UNQUOTE_RELAX, but in this case keep the extra backslash in the output) or in the middle of a sequence string. Add unit test cases to ensure the new flag works as expected and to prevent regressions from being introduced. Tested with a follow up commit converting config_parse_exec() to start using unquote_first_word, in which case this flags makes it possible to preserve unrecognized escape sequences. Relevant bug: https://bugs.freedesktop.org/show_bug.cgi?id=90794
It will try to unquot_first_word, but if it runs into escaping problems it will retry it adding UNQUOTE_CUNESCAPE_RELAX to the flags. If it succeeds on the second try, it will log a warning about it. If it fails both times, it will log an error. Add test cases to confirm it behaves as expected.
These tests will be useful to check the cases regarding quoted and escaped semicolon when we switch to using unquote_first_word. Additionally, convert some of the tests that have semicolons so that the argument after the semicolon looks like a path (starts with /) so that we can see the change of behavior when making config_parse_exec more strict about what it accepts as a command separator.
Convert config_parse_exec() from using FOREACH_WORD_QUOTED into a loop of unquote_first_word. Loop through the arguments only once (the FOREACH_WORD_QUOTED implementation did it twice, once to count them and another time to process and store them.) Use newly introduced flag UNQUOTE_UNESCAPE_RELAX to preserve unrecognized escape sequences such as regexps matches such as "\w", "\d", etc. (Valid escape sequences such as "\s" or "\b" still need an extra backslash if literals are desired for regexps.) Differences in behavior: - Handle ; (command separator) in special, so that only ; on its own is valid for that purpose, an quoted semicolon ";" or ';' will now behave as a literal semicolon. This is probably what was initially intended. - Handle \; (to introduce a literal semicolon) in special, so that only \; is turned into a semicolon but not \\; or "\\;" or "\;" which are kept as a literal \; in the output. This is probably what was initially intended. Known issues: - Using an empty string (for example, ExecStartPre=<empty>) will empty the list and remove the existing commands, but using whitespace only (for example, ExecStartPre=<spaces>) will not. This is a pre-existing issue and will be dealt with in a follow up commit. Tested: - Unit tests passing. Also `make distcheck` still works as expected. - Installed it on a local machine and booted with it, checked console output, systemctl and journalctl output, did not notice any issues running the patched systemd binaries. Relevant bug: https://bugs.freedesktop.org/show_bug.cgi?id=90794
…pace This is consistent with how an empty string works in an ExecStart= statement. We should not differentiate between an empty string and whitespace only (since they look the same.) Update the test case with whitespace only to reflect that the list is reset. Tested that `test-unit-file` passes and other test cases are not affected. Installed the patched systemd binaries on a machine, booted it, looked for out of the usual behavior but did not find any.
filbranden
force-pushed
the
unquote_first_word1
branch
from
a475d28 to
c83f1f3
Compare
|
Rebased (no conflicts, git did all the job by itself), retested with unit tests, also installed it and booted my machine with it, looked at output of So I'd say it's good to go. Cheers, |
Replace FOREACH_WORD_QUOTED with a loop using unquote_first_word in config_parse_exec()
|
Awesome thanks! @martinpitt Let me know if you find any regressions, hopefully the test coverage is enough to prove this works as supposed to and the minor changes in behavior of corner cases shouldn't really affect any real use cases. |
|
FTR, I turned the crank on my "build packages from upstream trunk" machinery again, and it passed both our integration tests as well as a manual one on a standard desktop install. We don't use any "fancy" Exec* statements there, but at least any potential regression shouldn't be so bad that we break the boot or anything. Sorry, I'm maybe just being a bit over-cautious after the previous trouble :-) |
|
@martinpitt Totally understandable. On the other hand, at one point or another this will end up hitting distros and that's when we're likely to find any problems, so whether it would be v221 or v222 doesn't seem to matter that much... Probably better to push it earlier since then we figure out if there's a problem earlier and deal with it, possibly in time to provide distros with a nice backport to fix it downstream. Cheers! |
|
@martinpitt thanks a lot! of course best would be to make that machinery run continously! the more CI the merrier! And we'll promise from upstream to look at it all the time. |
|
@poettering: absolutely. Right now I run it on my laptop daily, and upload the generated packages to https://launchpad.net/~pitti/+archive/ubuntu/systemd (this will only get packages whose tests succeeded). So other users can easily try trunk. This surely ought to run in some cloud instances, I'm working on that. But this might still be a few weeks, this is much more demanding than a |
Bz1256858 additional fix
networkd: make sure we remove udev fd from epoll *before* closing it
4 participants
@mbiebl @zonque @mezcalero
I'm fairly happy with this patch set and I'm fairly confident it fixes most of the recent quoting/escaping regressions.
The commit description indicates differences from previous versions, for instance handling of ; and ; is more strict now, please confirm you're OK with those changes.
Cheers,
Filipe