[systemd-devel] [RFC] load-fragment: use unquote_first_word in config_parse_exec

[Filipe Brandenburger]

Hi,

On Sun, May 31, 2015 at 12:06 AM, Andrei Borzenkov <arvidjaar at gmail.com> wrote:
> В Sat, 30 May 2015 23:29:29 -0700 Filipe Brandenburger <filbranden at google.com> пишет:
>> - 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