From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>

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@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
 src/libvirt_private.syms |   2 +
 src/util/virutil.c       | 173 +++++++++++++++++++++++++++++++++++++++
 src/util/virutil.h       |  18 ++++
 tests/utiltest.c         | 130 +++++++++++++++++++++++++++++
 4 files changed, 323 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 935ef7303b..25b22a3e3f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode;
 virHostHasIOMMU;
 virIndexToDiskName;
 virIsDevMapperDevice;
+virKernelCmdlineGetValue;
+virKernelCmdlineMatchParam;
 virMemoryLimitIsSet;
 virMemoryLimitTruncate;
 virMemoryMaxValue;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index fb46501142..264aae8d01 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void)
     return ret;
 }
 
+
+#define VIR_IS_CMDLINE_SPACE(value) \
+    (g_ascii_isspace(value) || (unsigned char) value == 160)
+
+
+static bool virKernelArgsAreEqual(const char *arg1,
+                                  const char *arg2,
+                                  size_t n)
+{
+    size_t i;
+
+    for (i = 0; i < n; i++) {
+        if ((arg1[i] == '-' && arg2[i] == '_') ||
+            (arg1[i] == '_' && arg2[i] == '-') || (arg1[i] == arg2[i])) {
+            if (arg1[i] == '\0')
+                return true;
+            continue;
+        }
+        return false;
+    }
+    return true;
+}
+
+
+/*
+ * Parse the kernel cmdline and store the value of @arg in @val
+ * which can be NULL if @arg has no value or if it's not found.
+ * In addition, store in @next the address right after
+ * @arg=@value for possible further processing.
+ *
+ * @arg: kernel command line argument
+ * @cmdline: kernel command line string to be checked for @arg
+ * @val: pointer to hold retrieved value of @arg
+ * @next: pointer to hold address right after @arg=@val, will
+ * point to the string's end (NULL) in case @arg is not found
+ *
+ * Returns 0 if @arg is present in @cmdline, 1 otherwise
+ */
+int virKernelCmdlineGetValue(const char *arg,
+                             const char *cmdline,
+                             char **val,
+                             const char **next)
+{
+    size_t i = 0, param_i;
+    size_t arg_len = strlen(arg);
+    bool is_quoted, match;
+
+    *next = cmdline;
+    *val = NULL;
+
+    while (cmdline[i] != '\0') {
+        /* remove leading spaces */
+        while (VIR_IS_CMDLINE_SPACE(cmdline[i]))
+            i++;
+        if (cmdline[i] == '"') {
+            i++;
+            is_quoted = true;
+        } else {
+            is_quoted = false;
+        }
+        for (param_i = i; cmdline[param_i]; param_i++) {
+            if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) ||
+                cmdline[param_i] == '=')
+                break;
+            if (cmdline[param_i] == '"')
+                is_quoted = !is_quoted;
+        }
+        if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg, arg_len))
+            match = true;
+        else
+            match = false;
+        /* arg has no value */
+        if (cmdline[param_i] != '=') {
+            if (match) {
+                *next = cmdline+param_i;
+                return 0;
+            }
+            i = param_i;
+            continue;
+        }
+        param_i++;
+        if (cmdline[param_i] == '\0')
+            break;
+
+        if (cmdline[param_i] == '"') {
+            param_i++;
+            is_quoted = !is_quoted;
+        }
+        i = param_i;
+
+        for (; cmdline[param_i]; param_i++) {
+            if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted)
+                break;
+            if (cmdline[param_i] == '"')
+                is_quoted = !is_quoted;
+        }
+        if (match) {
+            *next = cmdline+param_i;
+            if (cmdline[param_i-1] == '"')
+                *val = g_strndup(cmdline+i, param_i-i-1);
+            else
+                *val = g_strndup(cmdline+i, param_i-i);
+            return 0;
+        }
+        i = param_i;
+    }
+    *next = cmdline+i;
+    return 1;
+}
+
+
+#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
+ *   (uses size of value provided as input)
+ * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison
+ * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
+ *   (in case of multiple argument occurrences)
+ * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST: use the result of last argument occurence
+ *   (in case of multiple argument occurrences)
+ *
+ *
+ * @cmdline: kernel command line string to be checked for @arg
+ * @arg: kernel command line argument
+ * @values: array of possible values to match @arg
+ * @len_values: size of array, it can be 0 meaning a match will be positive if the
+ * argument has no value.
+ * @flags: flag mask defining the strategy for matching and comparing
+ *
+ * Returns true if a match is found, false otherwise
+ */
+bool virKernelCmdlineMatchParam(const char *cmdline,
+                                const char *arg,
+                                const char **values,
+                                size_t len_values,
+                                virKernelCmdlineFlags flags)
+{
+    bool match = false;
+    size_t i;
+    const char *next = cmdline;
+    g_autofree char *kval = NULL;
+
+    while (next[0] != '\0') {
+        VIR_FREE(kval);
+        if (virKernelCmdlineGetValue(arg, next, &kval, &next) != 0)
+            break;
+        if (!kval) {
+            match = (len_values == 0) ? true : false;
+        } else {
+            match = false;
+            for (i = 0; i < len_values; i++) {
+                if (VIR_CMDLINE_STR_CMP(kval, values[i], flags)) {
+                    match = true;
+                    break;
+                }
+            }
+        }
+        if (match && (flags & VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY))
+            break;
+    }
+
+    return match;
+}
+
+
 /*
  * Get a password from the console input stream.
  * The caller must free the returned password.
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 49b4bf440f..1edb415fbe 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -147,6 +147,24 @@ bool virHostHasIOMMU(void);
 
 char *virHostGetDRMRenderNode(void) G_GNUC_NO_INLINE;
 
+typedef enum {
+    VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX = 1,
+    VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ = 2,
+    VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY = 4,
+    VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST = 8,
+} virKernelCmdlineFlags;
+
+int virKernelCmdlineGetValue(const char *arg,
+                             const char *cmdline,
+                             char **val,
+                             const char **next);
+
+bool virKernelCmdlineMatchParam(const char *cmdline,
+                                const char *arg,
+                                const char **values,
+                                size_t len_values,
+                                virKernelCmdlineFlags flags);
+
 /**
  * VIR_ASSIGN_IS_OVERFLOW:
  * @rvalue: value that is checked (evaluated twice)
diff --git a/tests/utiltest.c b/tests/utiltest.c
index 5ae04585cb..1edc6a728e 100644
--- a/tests/utiltest.c
+++ b/tests/utiltest.c
@@ -254,6 +254,134 @@ testOverflowCheckMacro(const void *data G_GNUC_UNUSED)
 }
 
 
+struct testKernelCmdlineGetValueData
+{
+    const char *cmdline;
+    const char *arg;
+    int rc;
+    const char *val;
+    size_t next;
+};
+
+static struct testKernelCmdlineGetValueData kEntries[] = {
+    { "arg1 arg2 arg3=val1",                            "arg4",           1, NULL,                19 },
+    { "arg1=val1 arg2 arg3=val3 arg4",                  "arg2",           0, NULL,                14 },
+    { "arg1=val1 arg2 arg3=val3 arg4",                  "arg3",           0, "val3",              24 },
+    { "arg1=val1 arg2 arg-3=val3 arg4",                 "arg_3",          0, "val3",              25 },
+    { "arg1=val1 arg2 arg_3=val3 arg4",                 "arg-3",          0, "val3",              25 },
+    { "arg1=val1 arg2 arg_3=val3 arg4",                 "arg_3",          0, "val3",              25 },
+    { "arg1=val1 arg2 arg-3=val3 arg4",                 "arg-3",          0, "val3",              25 },
+    { "arg1=val1 arg2=\"value with spaces\" arg3=val3", "arg2",           0, "value with spaces", 34 },
+    { "arg1=val1 arg2=\"value with spaces\" arg3=val3", "arg3",           0, "val3",              44 },
+    { "arg1=val1 \"arg2=value with spaces\" arg3=val3", "arg2",           0, "value with spaces", 34 },
+    { "arg1=val1 \"arg2=value with spaces\" arg3=val3", "arg3",           0, "val3",              44 },
+    { "arg1=val1 arg2=\"val\"ue arg3",                  "arg2",           0, "val\"ue",           22 },
+    { "arg1=val1 arg2=\"val\"ue arg3\" escaped=val2\"", "arg3\" escaped", 0, "val2",              42 },
+    { "arg1=val1 arg2longer=someval arg2=val2 arg3",    "arg2",           0, "val2",              38 },
+};
+
+static int
+testKernelCmdlineGetValue(const void *data G_GNUC_UNUSED)
+{
+    int rc;
+    char *val = NULL;
+    const char *next;
+    size_t i;
+
+    for (i = 0; i < G_N_ELEMENTS(kEntries); ++i) {
+        VIR_FREE(val);
+
+        rc = virKernelCmdlineGetValue(kEntries[i].arg, kEntries[i].cmdline,
+                                      &val, &next);
+
+        if (rc != kEntries[i].rc || STRNEQ_NULLABLE(val, kEntries[i].val) ||
+            (next - kEntries[i].cmdline) != kEntries[i].next) {
+            VIR_TEST_DEBUG("\nKernel cmdline [%s]", kEntries[i].cmdline);
+            VIR_TEST_DEBUG("Kernel argument [%s]", kEntries[i].arg);
+            VIR_TEST_DEBUG("Expect rc [%d]", kEntries[i].rc);
+            VIR_TEST_DEBUG("Actual rc [%d]", rc);
+            VIR_TEST_DEBUG("Expect value [%s]", kEntries[i].val);
+            VIR_TEST_DEBUG("Actual value [%s]", val);
+            VIR_TEST_DEBUG("Expect next index [%lu]", kEntries[i].next);
+            VIR_TEST_DEBUG("Actual next index [%lu]",
+                           (size_t)(next - kEntries[i].cmdline));
+
+            VIR_FREE(val);
+
+            return -1;
+        }
+    }
+
+    VIR_FREE(val);
+
+    return 0;
+}
+
+
+struct testKernelCmdlineMatchData
+{
+    const char *cmdline;
+    const char *arg;
+    const char *values[2];
+    virKernelCmdlineFlags flags;
+    bool result;
+};
+
+static struct testKernelCmdlineMatchData kMatchEntries[] = {
+    {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"1", "y"},    VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ,     false },
+    {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"on", "yes"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ,     true },
+    {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"1", "y"},    VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX, true },
+    {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"a", "b"},    VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX, false },
+    {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"on", "yes"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ,       false},
+    {"arg1 myarg=no arg2=val2 myarg=yes arg4=val4 myarg=no arg5", "myarg", {"1", "y"},    VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX,   false},
+    {"arg1 myarg=no arg2=val2 arg4=val4 myarg=yes arg5",          "myarg", {"on", "yes"}, VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ,       true },
+    {"arg1 myarg=no arg2=val2 arg4=val4 myarg=yes arg5",          "myarg", {"1", "y"},    VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST | VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX,   true },
+    {"arg1 myarg=no arg2=val2 arg4=val4 myarg arg5",              "myarg", {NULL, NULL},  VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST,                                         true },
+    {"arg1 myarg arg2=val2 arg4=val4 myarg=yes arg5",             "myarg", {NULL, NULL},  VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY,                                       true },
+    {"arg1 myarg arg2=val2 arg4=val4 myarg=yes arg5",             "myarg", {NULL, NULL},  VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST,                                         false },
+};
+
+
+static int
+testKernelCmdlineMatchParam(const void *data G_GNUC_UNUSED)
+{
+    bool result;
+    size_t i, lenValues;
+
+    for (i = 0; i < G_N_ELEMENTS(kMatchEntries); ++i) {
+        if (kMatchEntries[i].values[0] == NULL)
+            lenValues = 0;
+        else
+            lenValues = G_N_ELEMENTS(kMatchEntries[i].values);
+
+        result = virKernelCmdlineMatchParam(kMatchEntries[i].cmdline,
+                                            kMatchEntries[i].arg,
+                                            kMatchEntries[i].values,
+                                            lenValues,
+                                            kMatchEntries[i].flags);
+
+        if (result != kMatchEntries[i].result) {
+            VIR_TEST_DEBUG("\nKernel cmdline [%s]", kMatchEntries[i].cmdline);
+            VIR_TEST_DEBUG("Kernel argument [%s]", kMatchEntries[i].arg);
+            VIR_TEST_DEBUG("Kernel values [%s] [%s]", kMatchEntries[i].values[0],
+                           kMatchEntries[i].values[1]);
+            if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX)
+                VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX]");
+            if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ)
+                VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ]");
+            if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY)
+                VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY]");
+            if (kMatchEntries[i].flags & VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST)
+                VIR_TEST_DEBUG("Flag [VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST]");
+            VIR_TEST_DEBUG("Expect result [%d]", kMatchEntries[i].result);
+            VIR_TEST_DEBUG("Actual result [%d]", result);
+
+            return -1;
+        }
+    }
+
+    return 0;
+}
 
 
 static int
@@ -277,6 +405,8 @@ mymain(void)
     DO_TEST(ParseVersionString);
     DO_TEST(RoundValueToPowerOfTwo);
     DO_TEST(OverflowCheckMacro);
+    DO_TEST(KernelCmdlineGetValue);
+    DO_TEST(KernelCmdlineMatchParam);
 
     return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
2.25.1

On Mon, May 11, 2020 at 06:41:56PM +0200, Boris Fiuczynski wrote:
> From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
> 
> 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@linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---
>  src/libvirt_private.syms |   2 +
>  src/util/virutil.c       | 173 +++++++++++++++++++++++++++++++++++++++
>  src/util/virutil.h       |  18 ++++
>  tests/utiltest.c         | 130 +++++++++++++++++++++++++++++
>  4 files changed, 323 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 935ef7303b..25b22a3e3f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode;
>  virHostHasIOMMU;
>  virIndexToDiskName;
>  virIsDevMapperDevice;
> +virKernelCmdlineGetValue;
> +virKernelCmdlineMatchParam;
>  virMemoryLimitIsSet;
>  virMemoryLimitTruncate;
>  virMemoryMaxValue;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index fb46501142..264aae8d01 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void)
>      return ret;
>  }
>  
> +
> +#define VIR_IS_CMDLINE_SPACE(value) \
> +    (g_ascii_isspace(value) || (unsigned char) value == 160)

Hmm, we're not checking the non-breaking space anywhere else in the code, so I
think we'd be fine not checking it here either, so plain g_ascii_isspace()
would suffice in the code

> +
> +
> +static bool virKernelArgsAreEqual(const char *arg1,
> +                                  const char *arg2,
> +                                  size_t n)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < n; i++) {
> +        if ((arg1[i] == '-' && arg2[i] == '_') ||
> +            (arg1[i] == '_' && arg2[i] == '-') || (arg1[i] == arg2[i])) {

Because '-' and '_' are equal in the parameter syntax, rather than introducing
another string equality function just because of this, we should normalize the
inputs by replacing occurrences of one with the other and then simply do STREQ
at the appropriate spot in the code


> +            if (arg1[i] == '\0')
> +                return true;
> +            continue;
> +        }
> +        return false;
> +    }
> +    return true;
> +}
> +
> +
> +/*
> + * Parse the kernel cmdline and store the value of @arg in @val
> + * which can be NULL if @arg has no value or if it's not found.
> + * In addition, store in @next the address right after
> + * @arg=@value for possible further processing.
> + *
> + * @arg: kernel command line argument
> + * @cmdline: kernel command line string to be checked for @arg
> + * @val: pointer to hold retrieved value of @arg
> + * @next: pointer to hold address right after @arg=@val, will
> + * point to the string's end (NULL) in case @arg is not found
> + *
> + * Returns 0 if @arg is present in @cmdline, 1 otherwise
> + */
> +int virKernelCmdlineGetValue(const char *arg,
> +                             const char *cmdline,
> +                             char **val,
> +                             const char **next)
> +{
> +    size_t i = 0, param_i;

in this specific case I think that naming the iteration variable param_i is
more confusing than actually settling down with something like "offset"
especially when you're doing arithmetics with it.

> +    size_t arg_len = strlen(arg);
> +    bool is_quoted, match;

1 declaration/definition per line please

> +
> +    *next = cmdline;
> +    *val = NULL;
> +
> +    while (cmdline[i] != '\0') {
> +        /* remove leading spaces */
> +        while (VIR_IS_CMDLINE_SPACE(cmdline[i]))
> +            i++;

For ^this, we already have a primitive virSkipSpaces()

> +        if (cmdline[i] == '"') {
> +            i++;
> +            is_quoted = true;
> +        } else {
> +            is_quoted = false;
> +        }
> +        for (param_i = i; cmdline[param_i]; param_i++) {
> +            if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) ||
> +                cmdline[param_i] == '=')
> +                break;
> +            if (cmdline[param_i] == '"')
> +                is_quoted = !is_quoted;
> +        }
> +        if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg, arg_len))

We don't use Yoda conditions, so ^this should be arg_len == param_i - i

> +            match = true;
> +        else
> +            match = false;
> +        /* arg has no value */
> +        if (cmdline[param_i] != '=') {
> +            if (match) {
> +                *next = cmdline+param_i;

please use a space in between the operands and the operator

> +                return 0;
> +            }
> +            i = param_i;
> +            continue;
> +        }
> +        param_i++;
> +        if (cmdline[param_i] == '\0')
> +            break;
> +
> +        if (cmdline[param_i] == '"') {
> +            param_i++;
> +            is_quoted = !is_quoted;
> +        }
> +        i = param_i;
> +
> +        for (; cmdline[param_i]; param_i++) {
> +            if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted)
> +                break;
> +            if (cmdline[param_i] == '"')
> +                is_quoted = !is_quoted;
> +        }
> +        if (match) {
> +            *next = cmdline+param_i;
> +            if (cmdline[param_i-1] == '"')
> +                *val = g_strndup(cmdline+i, param_i-i-1);
> +            else
> +                *val = g_strndup(cmdline+i, param_i-i);
> +            return 0;
> +        }
> +        i = param_i;
> +    }
> +    *next = cmdline+i;
> +    return 1;

Anyhow, this function is IMO doing too much on its own - parsing tokens,
matching the arguments, and extracting parameters and their values. All of that
should be split into helpers to enhance readability. Unfortunately you can't
use our existing tokenizer virStringSplit because of values containing spaces,
so you'll have to write your own that takes that into account, it should return
a token (parameter or parameter=value), then parse the returned token into
key-value pair, match the key with the input key and return whatever is needed
to be returned.

> +}
> +
> +
> +#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
> + *   (uses size of value provided as input)
> + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison
> + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search

Why are ^these constants needed? Especially the last two. Kernel is parsing the
parameters sequentially, so the last one wins. We should match that behaviour
when determining the value of a setting and ignore any previous occurrences of
a parameter.

Regards,
Erik

On 5/15/20 4:19 PM, Erik Skultety wrote:
> On Mon, May 11, 2020 at 06:41:56PM +0200, Boris Fiuczynski wrote:
>> From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
>>
>> 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@linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>>   src/libvirt_private.syms |   2 +
>>   src/util/virutil.c       | 173 +++++++++++++++++++++++++++++++++++++++
>>   src/util/virutil.h       |  18 ++++
>>   tests/utiltest.c         | 130 +++++++++++++++++++++++++++++
>>   4 files changed, 323 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 935ef7303b..25b22a3e3f 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode;
>>   virHostHasIOMMU;
>>   virIndexToDiskName;
>>   virIsDevMapperDevice;
>> +virKernelCmdlineGetValue;
>> +virKernelCmdlineMatchParam;
>>   virMemoryLimitIsSet;
>>   virMemoryLimitTruncate;
>>   virMemoryMaxValue;
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index fb46501142..264aae8d01 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void)
>>       return ret;
>>   }
>>   
>> +
>> +#define VIR_IS_CMDLINE_SPACE(value) \
>> +    (g_ascii_isspace(value) || (unsigned char) value == 160)
> 
> Hmm, we're not checking the non-breaking space anywhere else in the code, so I
> think we'd be fine not checking it here either, so plain g_ascii_isspace()
> would suffice in the code

Paulo wanted to make sure the function would always produce the same 
result as the kernel code itself and the kernel code recognizes 
character 160 as space.
In case you'd like to check that, see lib/cmdline.c -> function next_arg 
-> isspace -> include/linux/ctype.h -> __ismask & _S -> lib/ctype.c has 
a table with the entries for _S, including the character 160 which 
stands for "non breaking space" character.
Anyway since it seems an unlikely case I removed it as requested.

> 
>> +
>> +
>> +static bool virKernelArgsAreEqual(const char *arg1,
>> +                                  const char *arg2,
>> +                                  size_t n)
>> +{
>> +    size_t i;
>> +
>> +    for (i = 0; i < n; i++) {
>> +        if ((arg1[i] == '-' && arg2[i] == '_') ||
>> +            (arg1[i] == '_' && arg2[i] == '-') || (arg1[i] == arg2[i])) {
> 
> Because '-' and '_' are equal in the parameter syntax, rather than introducing
> another string equality function just because of this, we should normalize the
> inputs by replacing occurrences of one with the other and then simply do STREQ
> at the appropriate spot in the code

While reworking the parsing it has been changed.

> 
> 
>> +            if (arg1[i] == '\0')
>> +                return true;
>> +            continue;
>> +        }
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +
>> +/*
>> + * Parse the kernel cmdline and store the value of @arg in @val
>> + * which can be NULL if @arg has no value or if it's not found.
>> + * In addition, store in @next the address right after
>> + * @arg=@value for possible further processing.
>> + *
>> + * @arg: kernel command line argument
>> + * @cmdline: kernel command line string to be checked for @arg
>> + * @val: pointer to hold retrieved value of @arg
>> + * @next: pointer to hold address right after @arg=@val, will
>> + * point to the string's end (NULL) in case @arg is not found
>> + *
>> + * Returns 0 if @arg is present in @cmdline, 1 otherwise
>> + */
>> +int virKernelCmdlineGetValue(const char *arg,
>> +                             const char *cmdline,
>> +                             char **val,
>> +                             const char **next)
>> +{
>> +    size_t i = 0, param_i;
> 
> in this specific case I think that naming the iteration variable param_i is
> more confusing than actually settling down with something like "offset"
> especially when you're doing arithmetics with it.

Changed it.

> 
>> +    size_t arg_len = strlen(arg);
>> +    bool is_quoted, match;
> 
> 1 declaration/definition per line please

Changed it.

> 
>> +
>> +    *next = cmdline;
>> +    *val = NULL;
>> +
>> +    while (cmdline[i] != '\0') {
>> +        /* remove leading spaces */
>> +        while (VIR_IS_CMDLINE_SPACE(cmdline[i]))
>> +            i++;
> 
> For ^this, we already have a primitive virSkipSpaces()

Changed it.

> 
>> +        if (cmdline[i] == '"') {
>> +            i++;
>> +            is_quoted = true;
>> +        } else {
>> +            is_quoted = false;
>> +        }
>> +        for (param_i = i; cmdline[param_i]; param_i++) {
>> +            if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) ||
>> +                cmdline[param_i] == '=')
>> +                break;
>> +            if (cmdline[param_i] == '"')
>> +                is_quoted = !is_quoted;
>> +        }
>> +        if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg, arg_len))
> 
> We don't use Yoda conditions, so ^this should be arg_len == param_i - i

Corrected

> 
>> +            match = true;
>> +        else
>> +            match = false;
>> +        /* arg has no value */
>> +        if (cmdline[param_i] != '=') {
>> +            if (match) {
>> +                *next = cmdline+param_i;
> 
> please use a space in between the operands and the operator

Corrected

> 
>> +                return 0;
>> +            }
>> +            i = param_i;
>> +            continue;
>> +        }
>> +        param_i++;
>> +        if (cmdline[param_i] == '\0')
>> +            break;
>> +
>> +        if (cmdline[param_i] == '"') {
>> +            param_i++;
>> +            is_quoted = !is_quoted;
>> +        }
>> +        i = param_i;
>> +
>> +        for (; cmdline[param_i]; param_i++) {
>> +            if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted)
>> +                break;
>> +            if (cmdline[param_i] == '"')
>> +                is_quoted = !is_quoted;
>> +        }
>> +        if (match) {
>> +            *next = cmdline+param_i;
>> +            if (cmdline[param_i-1] == '"')
>> +                *val = g_strndup(cmdline+i, param_i-i-1);
>> +            else
>> +                *val = g_strndup(cmdline+i, param_i-i);
>> +            return 0;
>> +        }
>> +        i = param_i;
>> +    }
>> +    *next = cmdline+i;
>> +    return 1;
> 
> Anyhow, this function is IMO doing too much on its own - parsing tokens,
> matching the arguments, and extracting parameters and their values. All of that
> should be split into helpers to enhance readability. Unfortunately you can't
> use our existing tokenizer virStringSplit because of values containing spaces,
> so you'll have to write your own that takes that into account, it should return
> a token (parameter or parameter=value), then parse the returned token into
> key-value pair, match the key with the input key and return whatever is needed
> to be returned.

v2 will have a reworked approach which is more like you outlined above.

> 
>> +}
>> +
>> +
>> +#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
>> + *   (uses size of value provided as input)
>> + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison
>> + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
> 
> Why are ^these constants needed? Especially the last two. Kernel is parsing the
> parameters sequentially, so the last one wins. We should match that behaviour
> when determining the value of a setting and ignore any previous occurrences of
> a parameter.
> 
> Regards,
> Erik
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

On 15/05/20 16:19, Erik Skultety wrote:
> On Mon, May 11, 2020 at 06:41:56PM +0200, Boris Fiuczynski wrote:
>> From: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>
>>
>> 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@linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>>   src/libvirt_private.syms |   2 +
>>   src/util/virutil.c       | 173 +++++++++++++++++++++++++++++++++++++++
>>   src/util/virutil.h       |  18 ++++
>>   tests/utiltest.c         | 130 +++++++++++++++++++++++++++++
>>   4 files changed, 323 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 935ef7303b..25b22a3e3f 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -3428,6 +3428,8 @@ virHostGetDRMRenderNode;
>>   virHostHasIOMMU;
>>   virIndexToDiskName;
>>   virIsDevMapperDevice;
>> +virKernelCmdlineGetValue;
>> +virKernelCmdlineMatchParam;
>>   virMemoryLimitIsSet;
>>   virMemoryLimitTruncate;
>>   virMemoryMaxValue;
>> diff --git a/src/util/virutil.c b/src/util/virutil.c
>> index fb46501142..264aae8d01 100644
>> --- a/src/util/virutil.c
>> +++ b/src/util/virutil.c
>> @@ -1725,6 +1725,179 @@ virHostGetDRMRenderNode(void)
>>       return ret;
>>   }
>>   
>> +
>> +#define VIR_IS_CMDLINE_SPACE(value) \
>> +    (g_ascii_isspace(value) || (unsigned char) value == 160)
> 
> Hmm, we're not checking the non-breaking space anywhere else in the code, so I
> think we'd be fine not checking it here either, so plain g_ascii_isspace()
> would suffice in the code
> 
>> +
>> +
>> +static bool virKernelArgsAreEqual(const char *arg1,
>> +                                  const char *arg2,
>> +                                  size_t n)
>> +{
>> +    size_t i;
>> +
>> +    for (i = 0; i < n; i++) {
>> +        if ((arg1[i] == '-' && arg2[i] == '_') ||
>> +            (arg1[i] == '_' && arg2[i] == '-') || (arg1[i] == arg2[i])) {
> 
> Because '-' and '_' are equal in the parameter syntax, rather than introducing
> another string equality function just because of this, we should normalize the
> inputs by replacing occurrences of one with the other and then simply do STREQ
> at the appropriate spot in the code
> 
> 
>> +            if (arg1[i] == '\0')
>> +                return true;
>> +            continue;
>> +        }
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +
>> +/*
>> + * Parse the kernel cmdline and store the value of @arg in @val
>> + * which can be NULL if @arg has no value or if it's not found.
>> + * In addition, store in @next the address right after
>> + * @arg=@value for possible further processing.
>> + *
>> + * @arg: kernel command line argument
>> + * @cmdline: kernel command line string to be checked for @arg
>> + * @val: pointer to hold retrieved value of @arg
>> + * @next: pointer to hold address right after @arg=@val, will
>> + * point to the string's end (NULL) in case @arg is not found
>> + *
>> + * Returns 0 if @arg is present in @cmdline, 1 otherwise
>> + */
>> +int virKernelCmdlineGetValue(const char *arg,
>> +                             const char *cmdline,
>> +                             char **val,
>> +                             const char **next)
>> +{
>> +    size_t i = 0, param_i;
> 
> in this specific case I think that naming the iteration variable param_i is
> more confusing than actually settling down with something like "offset"
> especially when you're doing arithmetics with it.
> 
>> +    size_t arg_len = strlen(arg);
>> +    bool is_quoted, match;
> 
> 1 declaration/definition per line please
> 
>> +
>> +    *next = cmdline;
>> +    *val = NULL;
>> +
>> +    while (cmdline[i] != '\0') {
>> +        /* remove leading spaces */
>> +        while (VIR_IS_CMDLINE_SPACE(cmdline[i]))
>> +            i++;
> 
> For ^this, we already have a primitive virSkipSpaces()
> 
>> +        if (cmdline[i] == '"') {
>> +            i++;
>> +            is_quoted = true;
>> +        } else {
>> +            is_quoted = false;
>> +        }
>> +        for (param_i = i; cmdline[param_i]; param_i++) {
>> +            if ((VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted) ||
>> +                cmdline[param_i] == '=')
>> +                break;
>> +            if (cmdline[param_i] == '"')
>> +                is_quoted = !is_quoted;
>> +        }
>> +        if (param_i-i == arg_len && virKernelArgsAreEqual(cmdline+i, arg, arg_len))
> 
> We don't use Yoda conditions, so ^this should be arg_len == param_i - i
> 
>> +            match = true;
>> +        else
>> +            match = false;
>> +        /* arg has no value */
>> +        if (cmdline[param_i] != '=') {
>> +            if (match) {
>> +                *next = cmdline+param_i;
> 
> please use a space in between the operands and the operator
> 
>> +                return 0;
>> +            }
>> +            i = param_i;
>> +            continue;
>> +        }
>> +        param_i++;
>> +        if (cmdline[param_i] == '\0')
>> +            break;
>> +
>> +        if (cmdline[param_i] == '"') {
>> +            param_i++;
>> +            is_quoted = !is_quoted;
>> +        }
>> +        i = param_i;
>> +
>> +        for (; cmdline[param_i]; param_i++) {
>> +            if (VIR_IS_CMDLINE_SPACE(cmdline[param_i]) && !is_quoted)
>> +                break;
>> +            if (cmdline[param_i] == '"')
>> +                is_quoted = !is_quoted;
>> +        }
>> +        if (match) {
>> +            *next = cmdline+param_i;
>> +            if (cmdline[param_i-1] == '"')
>> +                *val = g_strndup(cmdline+i, param_i-i-1);
>> +            else
>> +                *val = g_strndup(cmdline+i, param_i-i);
>> +            return 0;
>> +        }
>> +        i = param_i;
>> +    }
>> +    *next = cmdline+i;
>> +    return 1;
> 
> Anyhow, this function is IMO doing too much on its own - parsing tokens,
> matching the arguments, and extracting parameters and their values. All of that
> should be split into helpers to enhance readability. Unfortunately you can't
> use our existing tokenizer virStringSplit because of values containing spaces,
> so you'll have to write your own that takes that into account, it should return
> a token (parameter or parameter=value), then parse the returned token into
> key-value pair, match the key with the input key and return whatever is needed
> to be returned.
> 
>> +}
>> +
>> +
>> +#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
>> + *   (uses size of value provided as input)
>> + * - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ: do a strict string comparison
>> + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_STICKY: first positive match satifies search
> 
> Why are ^these constants needed? Especially the last two. Kernel is parsing the
> parameters sequentially, so the last one wins. We should match that behaviour
> when determining the value of a setting and ignore any previous occurrences of
> a parameter.

It's not always the case that the last one wins. In the case of the 
prot_virt parameter for example as soon as a positive value (prot_virt=1 
or prot_virt=y) shows up in the kernel cmdline the feature will be 
enabled, even if the last entry is negative. In order to handle the two 
possible behaviors we introduced the *_SEARCH flags SEARCH_STICKY 
(prot_virt behavior) and SEARCH_LAST (usual behavior, last one wins).

As to the *_CMP flags, prot_virt again accepts multiple values and does 
a prefix-based check (see arch/s390/kernel/uv.c -> prot_virt_setup -> 
kstrtobool) so we introduced CMP_PREFIX to handle that, while CMP_EQ is 
for the usual case when the whole value is considered.

> 
> Regards,
> Erik
> 

-- 
Best regards,

Paulo de Rezende Pinatti