Extend `cgroup:` parsing to support values with spaces

[aanderse]

consider the following example:

aaron@framework ~> cat /sys/fs/cgroup/system/some-daemon/cpu.max
200000 100000

maybe we could support a syntax like this?

service cgroup:cpu.max:'200000 100000',pids.max:1048576 /usr/bin/some-daemon

what do you think? 🤔

[troglobit]

Yeah something like that would be very useful. However, I'm afraid there's quite a lot of changes needed to fix this. Not just service_register() needs to have it's tokenizer refactored, most of the cgroup.c code as well only handles single value arguments.

[aanderse]

ah, i see

[aanderse]

Yeah something like that would be very useful. However, I'm afraid there's quite a lot of changes needed to fix this. Not just service_register() needs to have it's tokenizer refactored, most of the cgroup.c code as well only handles single value arguments.

after reading through cgroup.c i wasn't convinced this statement is entirely true

i wrote a hacky little patch to prove to myself i was understanding the code correctly and i can confirm that only service.c would need to be modified to make this work

---

for reference i wrote a ridiculous little patch to basically bypass the service.c parsing issue by using | as a substitution for (space) in service.c, then substituted the space back in before passing to cgroup.c, like so:

diff --git a/src/service.c b/src/service.c
index 023b9bc1..79e3cd01 100644
--- a/src/service.c
+++ b/src/service.c
@@ -1271,6 +1271,11 @@ static void parse_cgroup(svc_t *svc, char *cgroup)
        }

        strlcpy(svc->cgroup.cfg, ptr, sizeof(svc->cgroup.cfg));
+       for (int i = 0; i < strlen(svc->cgroup.cfg); i++) {
+               if (svc->cgroup.cfg[i] == '|') {
+                       svc->cgroup.cfg[i] = ' ';
+               }
+       }
 }

 static void parse_sighalt(svc_t *svc, char *arg)

it was silly, but it was an incredibly quick way to validate my theory 🤷‍♂

every time i consider what would be the best solution here i keep finding myself looking at #148... 😓 though i suppose a simple strtok_r replacement would suffice for now?

[troglobit]

Nicely done! Very interesting, I only really skimmed through the code, so this is quite reassuring.

Maybe strtok_r() can help us, not sure though, but it's definitely worth a try.

[aanderse]

update: i wrote up a slightly modified strtok_r implementation which allows spaces in single quoted strings... looks promising

will get around to some testing and report back if it seems viable

[troglobit]

update: i wrote up a slightly modified strtok_r implementation which allows spaces in single quoted strings... looks promising

will get around to some testing and report back if it seems viable

Cool, looking forward to having a look! 💪😎

[troglobit]

@aanderse how did it turn out? I'm particularly curious now since a colleague expressed a need to add a reload:/path/to/foo.sh reload to the service stanza. (Seems this is what Frr 10.x needs these days.)

[aanderse]

i replaced the relevant strtok_r calls in service.c with a custom implementation which allows quoted strings and so far so good

unfortunately i haven't had time to clean this up for a patch yet - i will try to get around to it soon, but i can confirm the fix to this is relatively simple 👍

I'm particularly curious now since a colleague expressed a need to add a reload:/path/to/foo.sh reload to the service stanza.

awesome! i ran into a situation where a daemon reloads on SIGUSR2 instead of SIGHUP so this is welcome.

[troglobit]

Alright, I might go ahead anyway with something like this to replace strtok_t() calls:

static char *parse_args(char **line)
{
	char *start, *end = NULL, *arg;
	char in_quote = 0;
	int has_colon = 0;

	if (!line || !*line)
		return NULL;

	/* Skip leading whitespace */
	while (**line && (**line == ' ' || **line == '\t'))
		(*line)++;

	if (!**line)
		return NULL;

	start = *line;

	/* Parse the token */
	while (**line) {
		if (in_quote) {
			if (**line == in_quote) {
				/* End quote found */
				in_quote = 0;
			}
		} else {
			if (**line == '\'' || **line == '"') {
				/* Start quote */
				in_quote = **line;
			} else if (**line == ':') {
				/* Found colon - this might be key:value format */
				has_colon = 1;
			} else if (**line == ' ' || **line == '\t') {
				/* Whitespace - check if we're in key:value mode */
				if (has_colon) {
					/* Look ahead to see if there's a comma after whitespace */
					char *lookahead = *line;
					while (*lookahead && (*lookahead == ' ' || *lookahead == '\t'))
						lookahead++;
					if (*lookahead == ',') {
						/* Continue parsing - this space is part of the token */
						(*line)++;
						continue;
					}
				}
				/* End token at whitespace */
				end = *line;
				break;
			}
		}
		(*line)++;
	}

	/* Set end if we reached end of string */
	if (!end)
		end = *line;

	/* Null terminate the argument */
	if (end > start) {
		arg = start;
		if (*end) {
			*end = '\0';
			*line = end + 1;
		}
		return arg;
	}

	return NULL;
}

I also had to patch cgset() slightly:

diff --git a/src/cgroup.c b/src/cgroup.c
index 8e7a140..64f920e 100644
--- a/src/cgroup.c
+++ b/src/cgroup.c
@@ -86,6 +86,21 @@ static void cgset(const char *path, char *ctrl, char *prop)
        }
        *val++ = 0;
 
+       /* unquote value, if quoted */
+       if (val[0] == '"' || val[0] == '\'') {
+               char q = val[0];
+               char *end;
+
+               end = strchr(&val[1], q);
+               if (!end) {
+                       errx(1, "Syntax error, unterminated quote in %s/%s.%s=%s", path, ctrl, prop, val);
+                       return;
+               }
+
+               val++;
+               *end = 0;
+       }
+
        /* disallow sneaky relative paths */
        if (strstr(ctrl, "..") || strstr(prop, "..")) {
                errx(1, "Possible security violation; '..' not allowed in cgroup config!");

[aanderse]

in effect this is what i have yes!

i used strtok_r as the template but giving that some thought i guess the reentrant part isn't really necessary is it... 😅

you should go with what you have here... it didn't even occur to me to check for a colon and comma combo - that leads to a really nice option for users!

my only question is whether you should unquote text in your new function itself... otherwise won't you have to unquote not just in cgset but reload too, for example?

[troglobit]

in effect this is what i have yes!

Great, thanks for taking the time to respond! <3

my only question is whether you should unquote text in your new function itself... otherwise won't you have to unquote not just in cgset but reload too, for example?

I dunno, thought about it at first, but since it's basically up to each subsystem to define the syntax after their token/command, I think it's better to do the unquoting further down the tree. But I'll definitely break it out as a helper function, because that's already the third instance of the same so.

[aanderse]

thanks so much @troglobit!