Re: [PATCH v2 1/7] util: introduce a parser for kernel cmdline arguments

On 08/06/20 16:35, Erik Skultety wrote: > On Fri, May 29, 2020 at 12:10:03PM +0200, Paulo de Rezende Pinatti wrote: > > Introduce two utility functions to parse a kernel command > > line string according to the kernel code parsing rules in > > order to enable the caller to perform operations such as > > verifying whether certain argument=value combinations are > > present or retrieving an argument's value. > > > > Signed-off-by: Paulo de Rezende Pinatti <ppinatti(a)linux.ibm.com&gt; > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > > --- > > src/libvirt_private.syms | 2 + > > src/util/virutil.c | 169 +++++++++++++++++++++++++++++++++++++++ > > src/util/virutil.h | 17 ++++ > > tests/utiltest.c | 141 ++++++++++++++++++++++++++++++++ > > 4 files changed, 329 insertions(+) > > > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > > index a6af44fe1c..a206a943c5 100644 > > --- a/src/libvirt_private.syms > > +++ b/src/libvirt_private.syms > > @@ -3433,6 +3433,8 @@ virHostGetDRMRenderNode; > > virHostHasIOMMU; > > virIndexToDiskName; > > virIsDevMapperDevice; > > +virKernelCmdlineMatchParam; > > +virKernelCmdlineNextParam; > > virMemoryLimitIsSet; > > virMemoryLimitTruncate; > > virMemoryMaxValue; > > diff --git a/src/util/virutil.c b/src/util/virutil.c > > index fb46501142..749c9d7116 100644 > > --- a/src/util/virutil.c > > +++ b/src/util/virutil.c > > @@ -1725,6 +1725,175 @@ virHostGetDRMRenderNode(void) > > return ret; > > } > > > > + > > +static const char *virKernelCmdlineSkipDbQuote(const char *cmdline, > > minor nitpick: we can call ^this simply ...SkipQuote, we don't need to be so > specific about it being a double quotation mark, do we? > Sure, changed to SkipQuote. > > + bool *is_quoted) > > +{ > > + if (cmdline[0] == '"') { > > + *is_quoted = !(*is_quoted); > > + cmdline++; > > + } > > + return cmdline; > > +} > > + > > + > > +static size_t virKernelCmdlineSearchForward(const char *cmdline, > > + bool *is_quoted, > > + bool include_equal) > > Hmm, what if instead we tried to find and return the index of the '=' character > but iterated all the way until the next applicable (i.e. taking quotation into > account) space and saved that end of arg/arg=val parameter into **res. The > caller of this function would then continue directly from *res with the next > arg/arg=val parameter. > We could then call this something like virKernelCmdlineFindEqual and return -1 > if there is no '=' sign, indicating that it's a standalone parameter with no > value. > Yes, we can do that. It would look like this: size_t index;

size_t equal_index = 0; for (index = 0; cmdline[index]; index++) { if (!(is_quoted) && g_ascii_isspace(cmdline[index])) break; if (equal_index == 0 && cmdline[index] == '=') { equal_index = index; continue; } virKernelCmdlineSkipQuote(cmdline + index, &is_quoted); } *res = cmdline + index; return equal_index; I found it more convenient to use 0 rather than -1 so that we can also handle the case when the argument itself starts with the equal sign.

So the function would look like this now: virSkipSpaces(&cmdline); cmdline = virKernelCmdlineSkipQuote(cmdline, &is_quoted); equal_index = virKernelCmdlineFindEqual(cmdline, is_quoted, &next); if (next == cmdline) return next; /* param has no value */ if (equal_index == 0) { if (is_quoted && next[-1] == '"') *param = g_strndup(cmdline, next - cmdline - 1); else *param = g_strndup(cmdline, next - cmdline); return next; } *param = g_strndup(cmdline, equal_index); if (cmdline[equal_index + 1] == '"') { is_quoted = true; equal_index++; } if (is_quoted && next[-1] == '"') *val = g_strndup(cmdline + equal_index + 1, next - cmdline - equal_index - 2); else *val = g_strndup(cmdline + equal_index + 1, next - cmdline - equal_index - 1); return next; There's still a bit of handling required due to the kernel's quoting rules. I took the liberty of using 'next' in place of 'res' as I think it conveys better its purpose. Let me know if that looks good to you.

> > +} > > + > > + > > +#define VIR_CMDLINE_STR_CMP(kernel_val, caller_val, flags) \ > > + (((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ) && \ > > + STREQ(kernel_val, caller_val)) || ((flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX) && \ > > + STRPREFIX(kernel_val, caller_val))) > > + > > + > > +/* > > + * Try to match the provided kernel cmdline string with the provided @arg > > + * and the list @values of possible values according to the matching strategy > > + * defined in @flags. Possible options include: > > + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX: do a substring comparison of values > > I know you used it in the following patches to match against the accepted > values, but do we really need to match with a prefix, I'd be fine with simple > stirng equality matching in all cases and not mix the matching strategy for > both values and kernel arguments. > The prefix based comparison is to make sure libvirt matches exactly like the kernel code does for the parameter prot_virt (see arch/s390/kernel/uv.c -> prot_virt_setup -> kstrtobool) which performs prefix based matching. Using equality matching should work for most cases but there would be certain corner cases missed (i.e. prot_virt=yfoo will be recognized as 'activate' by the kernel but not by libvirt).

If you think such cases are not worth the trouble I can remove the PREFIX flag, let me know what you prefer.

> > + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search > > is "sticky searching" some terminus technicus? If not, we probably should name > this FLAGS_MATCH_FIRST and FLAGS_MATCH_LAST respectively. > I guess the flag explanation was not clear enough, sticky actually means "stop as soon as you find a match, no matter the order", so

FLAGS_SEARCH_FIRST doesn't really fit here. A better name could be FLAGS_SEARCH_ANY. Let me know if you agree.

> > > > +struct testKernelCmdlineNextParamData > > +{ > > + const char *cmdline; > > + const char *param; > > + const char *val; > > + const char *next; > > +}; > > + > > +static struct testKernelCmdlineNextParamData kEntries[] = { > > + { "arg1 arg2 arg3=val1", "arg1", NULL, " arg2 arg3=val1" }, > > + { "arg1=val1 arg2 arg3=val3 arg4", "arg1", "val1", " arg2 arg3=val3 arg4" }, > > + { "arg3=val3 ", "arg3", "val3", " " }, > > + { "arg3=val3", "arg3", "val3", "" }, > > + { "arg-3=val3 arg4", "arg-3", "val3", " arg4" }, > > + { "arg_3=val3 arg4", "arg-3", "val3", " arg4" }, > > + { " arg_3=val3 arg4", "arg-3", "val3", " arg4" }, > > + { " arg-3=val3 arg4", "arg-3", "val3", " arg4" }, > > + { "arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, > > + { " arg2=\"value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, > > + { " \"arg2=value with spaces\" arg3=val3", "arg2", "value with spaces", " arg3=val3" }, > > + { "arg2=\"val\"ue arg3", "arg2", "val\"ue", " arg3" }, > > + { " arg3\" escaped=val2\"", "arg3\" escaped", "val2", "" }, > > ^Is this even valid for the kernel itself? Looking at [1], they clearly don't > allow escaped \" in the arg/value. > > [1] https://github.com/torvalds/linux/blob/db54615e21419c3cb4d699a0b0aa16cc44... > I guess the word "escaped" in this test is a bit misleading; it's actually escaping the blank space, not the quote itself. This is valid for the kernel. In order to assure our parsing results would match those of the kernel I executed the code in cmdline.c in a standalone file to generate the reference values for the test. I'll change "arg3\" escaped" to "arg3\" with spaces" to clearly state the intention here.