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
