Hi, On Sun, May 31, 2015 at 12:06 AM, Andrei Borzenkov <[email protected]> wrote: > В Sat, 30 May 2015 23:29:29 -0700 Filipe Brandenburger > <[email protected]> пишет: >> - Handling a \; is ugly, it looks like a hack... unquote_first_word is >> not equipped to recognize that sequence, so I had to move it outside >> unquote_first_word looking for those two characters followed by >> whitespace explicitly. But then, something like ';' or ";" will be >> recognized as a command separator, is that OK? > > I do not think it's OK. Having quoted string act like a separator is > definitely unexpected and confusing.
I believe this matches the current behavior, having a single ";" or ';' become a command separator, only \; is recognized as a valid escape. The reason is that both FOREACH_WORD_QUOTED and unquote_first_word will simply return the unquoted string, so there is no good way to differentiate ; from ";" or ';'. We could work around that (after the conversion to unquote_first_word) similarly to how \; is being handled, by looking at the next characters of p to check if p[0] == ';' and p[1] is whitespace and handle that in particular. >> - We do something different for empty string (clear the command list) >> than we do for just whitespace. This is pre-existing. Maybe we need to >> fix that? I put a comment on that case, that branch is triggered both >> in the "just whitespace" case as well as right after a semicolon >> command separator. > > Not sure I understand what you mean. Do you suggest that > > ExecStart=/usr/bin/foo ; ; /usr/bin/bar > > should discard /usr/bin/foo? Or that it does it already? > > The use case is to clear any pre-existing value in drop-in like > > ExecStart= > ExecStart=new command line So, my read of the current code makes me believe that ExecStart=<empty> will indeed clear any pre-existing value but on the other hand ExecStart=<whitespace> will not. That's what I'm referring to. I'm not really sure how the current code behaves to two semicolons in a row, like your "/usr/bin/foo ; ; /usr/bin/bar" example. The code after my patch will probably try to handle the second ";" as a command name and will most likely choke on it at the very least because it's not an absolute path. I haven't really tested either in particular though... Sorry if I didn't make it clear... but in most cases the limitations I mentioned already existed in the old code. I just want to make sure that the new code is not more buggy or buggy in worse ways than the old code. I believe addressing these shortcomings in the new code should be more straightforward. In particular, I plan to start looking at the empty vs. whitespace issue and maybe have a follow up commit to this one that addresses that. Cheers, Filipe _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
