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

On Mon, Jun 15, 2020 at 10:28:06AM +0200, Paulo de Rezende Pinatti wrote: Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; The code is fine, I just spotted last minute codestyle nitpick that I didn't pay attention to previously, so if you're okay with the snippet below, I'll squash it in before merging: diff --git a/src/util/virutil.c b/src/util/virutil.c index 30df707b8e..b7ea643e4a 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1737,18 +1737,17 @@ static const char *virKernelCmdlineSkipQuote(const char *cmdline, } -/** - * virKernelCmdlineFindEqual: +/* + * Iterate over the provided kernel command line string while honoring + * the kernel quoting rules and returns the index of the equal sign + * separating argument and value. + * * @cmdline: target kernel command line string * @is_quoted: indicates whether the string begins with quotes * @res: pointer to the position immediately after the parsed parameter, * can be used in subsequent calls to process further parameters until * the end of the string. * - * Iterate over the provided kernel command line string while honoring - * the kernel quoting rules and returns the index of the equal sign - * separating argument and value. - * * Returns 0 for the cases where no equal sign is found or the argument * itself begins with the equal sign (both cases indicating that the * argument has no value). Otherwise, returns the index of the equal @@ -1781,17 +1780,16 @@ static char* virKernelArgNormalize(const char *arg) } -/** - * virKernelCmdlineNextParam: - * @cmdline: kernel command line string to be checked for next parameter - * @param: pointer to hold retrieved parameter, will be NULL if none found - * @val: pointer to hold retrieved value of @param - * +/* * Parse the kernel cmdline and store the next parameter in @param * and the value of @param in @val which can be NULL if @param has * no value. In addition returns the address right after @param=@value * for possible further processing. * + * @cmdline: kernel command line string to be checked for next parameter + * @param: pointer to hold retrieved parameter, will be NULL if none found + * @val: pointer to hold retrieved value of @param + * * Returns a pointer to address right after @param=@val in the * kernel command line, will point to the string's end (NULL) * in case no next parameter is found @@ -1849,19 +1847,25 @@ static bool virKernelCmdlineStrCmp(const char *kernel_val, } -/** - * virKernelCmdlineMatchParam: +/* + * 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 of values, + * this is the default if no CMP flag was specified + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_FIRST: first match satisfies search, no matter + * the order (in case of multiple argument occurrences) + * - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST: use the result of last argument occurrence + * (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: bitwise-OR of virKernelCmdlineFlags - * - * 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. - * + * @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 */ @@ -1880,12 +1884,10 @@ bool virKernelCmdlineMatchParam(const char *cmdline, g_autofree char *kparam = NULL; g_autofree char *kparam_norm = NULL; g_autofree char *kval = NULL; - next = virKernelCmdlineNextParam(next, &kparam, &kval); if (!kparam) break; - kparam_norm = virKernelArgNormalize(kparam); if (STRNEQ(kparam_norm, arg_norm)) diff --git a/src/util/virutil.h b/src/util/virutil.h index 0b9ef5b850..b1f6cfdfe4 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -147,12 +147,11 @@ bool virHostHasIOMMU(void); char *virHostGetDRMRenderNode(void) G_GNUC_NO_INLINE; -/* Kernel cmdline match and comparison strategy for arg=value pairs */ typedef enum { - VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX = 1, /* substring comparison of argument values */ - VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ = 2, /* strict string comparison of argument values */ - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_FIRST = 4, /* match first occurrence of pattern */ - VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST = 8, /* match last occurrence of pattern */ + VIR_KERNEL_CMDLINE_FLAGS_CMP_PREFIX = 1, + VIR_KERNEL_CMDLINE_FLAGS_CMP_EQ = 2, + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_FIRST = 4, + VIR_KERNEL_CMDLINE_FLAGS_SEARCH_LAST = 8, } virKernelCmdlineFlags; const char *virKernelCmdlineNextParam(const char *cmdline, diff --git a/tests/utiltest.c b/tests/utiltest.c index 2bff7859dc..e6c1bb1050 100644 --- a/tests/utiltest.c +++ b/tests/utiltest.c @@ -286,12 +286,14 @@ static struct testKernelCmdlineNextParamData kEntries[] = { static int testKernelCmdlineNextParam(const void *data G_GNUC_UNUSED) { + char *param = NULL; + char *val = NULL; const char *next; size_t i; for (i = 0; i < G_N_ELEMENTS(kEntries); ++i) { - g_autofree char * param = NULL; - g_autofree char * val = NULL; + VIR_FREE(param); + VIR_FREE(val); next = virKernelCmdlineNextParam(kEntries[i].cmdline, &param, &val); @@ -306,10 +308,16 @@ testKernelCmdlineNextParam(const void *data G_GNUC_UNUSED) VIR_TEST_DEBUG("Expect next [%s]", kEntries[i].next); VIR_TEST_DEBUG("Actual next [%s]", next); + VIR_FREE(param); + VIR_FREE(val); + return -1; } } + VIR_FREE(param); + VIR_FREE(val); + return 0; }