Re: [systemd-devel] [RFC] load-fragment: use unquote_first_word in config_parse_exec
В Wed, 3 Jun 2015 20:34:53 -0700 Filipe Brandenburger filbran...@google.com пишет: - 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? I added a test case to cover two semicolons in a row. With the new code it breaks as it thinks ; is a new command name, I think that's appropriate enough, let me know if you think otherwise. I cannot think where this would be useful, so I guess it is OK as long as error is clear enough. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] load-fragment: use unquote_first_word in config_parse_exec
Hi, This commit was not moved to GitHub, it's under review here: https://github.com/systemd/systemd/pull/44 On Sun, May 31, 2015 at 12:06 AM, Andrei Borzenkov arvidj...@gmail.com wrote: В Sat, 30 May 2015 23:29:29 -0700 Filipe Brandenburger filbran...@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. Addressed. Now ; or ';' will turn into a literal semicolon, so will \; only when it's on its own, and only a single ; on its own will be recognized as a command separator. - 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? I added a test case to cover two semicolons in a row. With the new code it breaks as it thinks ; is a new command name, I think that's appropriate enough, let me know if you think otherwise. Cheers! Filipe ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] load-fragment: use unquote_first_word in config_parse_exec
Hi, On Sun, May 31, 2015 at 12:06 AM, Andrei Borzenkov arvidj...@gmail.com wrote: В Sat, 30 May 2015 23:29:29 -0700 Filipe Brandenburger filbran...@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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [RFC] load-fragment: use unquote_first_word in config_parse_exec
This is an attempt to convert it from using FOREACH_WORD_QUOTED into a loop using unquote_first_word repeatedly. Additionally, we're looping through the arguments only once and using _cleanup_ functions to manage objects owned by this function. Some notes: - There is some difference in how unquote_first_word handles invalid quotes or escapes. In particular, for some cases where we used to return 0 (but set the command to NULL) we are now returning -EINVAL instead (still setting the command to NULL.) I don't know if that's a problem anywhere... - The test cases were updated to reflect that change. - 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? - 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. - Now we're logging more from this function, in particular if we run out of memory we return log_oom(), but that might not be the case for instance if we get -ENOMEM from unquote_first_word. Should we wrap around that if (r 0) return r; in something that logs errno? - Not sure whether we need to track semicolon on a variable of its own or if we can just check whether *p is the \0 end of the string. I noticed the original code had a case logging ... ignoring trailing garbage, but I'm not sure where that's needed... Do we need that here as well or is that something that will not happen with unquote_first_word? Tested it by running the test suite. I also built a systemd with this patch, installed and ran it successfully on a VM running Arch Linux. I didn't notice anything too weird (but maybe I just didn't look at all the right places...) Comments welcome! Cheers, Filipe --- src/core/load-fragment.c | 209 ++ src/test/test-unit-file.c | 14 +++- 2 files changed, 108 insertions(+), 115 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index c95c11014c72..dd2cf6e3257f 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -520,9 +520,9 @@ int config_parse_exec( void *data, void *userdata) { -ExecCommand **e = data, *nce; -char *path, **n; -unsigned k; +ExecCommand **e = data; +const char *p; +bool semicolon; int r; assert(filename); @@ -538,150 +538,137 @@ int config_parse_exec( return 0; } +rvalue += strspn(rvalue, WHITESPACE); +p = rvalue; + /* We accept an absolute path as first argument, or * alternatively an absolute prefixed with @ to allow * overriding of argv[0]. */ -for (;;) { +do { int i; -const char *word, *state, *reason; -size_t l; +_cleanup_strv_free_ char **n = NULL; +_cleanup_free_ ExecCommand *nce = NULL; +_cleanup_free_ char *path = NULL; +_cleanup_free_ char *firstword = NULL; +char *f; bool separate_argv0 = false, ignore = false; -path = NULL; -nce = NULL; -n = NULL; +semicolon = false; -rvalue += strspn(rvalue, WHITESPACE); +r = unquote_first_word(p, firstword, UNQUOTE_CUNESCAPE); +if (r 0) +return r; +if (r == 0) +/* We get here in two situations: + * 1. If we have only whitespace (shouldn't this reset + *the list same as an empty string?) + * 2. With a trailing semicolon and no command after that. + */ +return 0; -if (rvalue[0] == 0) -break; +f = firstword; +for (i = 0; i 2; i++) { +if (*f == '-' !ignore) { +ignore = true; +f ++; +} else if (*f == '@' !separate_argv0) { +separate_argv0 = true; +f ++; +} +} -k = 0; -FOREACH_WORD_QUOTED(word, l, rvalue, state) { -if (k == 0) { -for (i = 0; i 2; i++) { -if
Re: [systemd-devel] [RFC] load-fragment: use unquote_first_word in config_parse_exec
В Sat, 30 May 2015 23:29:29 -0700 Filipe Brandenburger filbran...@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. - 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel