Re: [PATCH v3 1/7] util: introduce a parser for kernel cmdline arguments
On Mon, Jun 15, 2020 at 04:49:10PM +0200, Paulo de Rezende Pinatti wrote:
On 15/06/20 16:16, Erik Skultety wrote:
> On Mon, Jun 15, 2020 at 10:28:06AM +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>
> > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
>
> Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
>
> 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:
I just think the description of the flag SEARCH_FIRST might not be clear
enough without the "no matter the order" text, as it might suggest it just
checks the first occurrence of the parameter and stops there (as opposed to
the flag SEARCH_LAST which checks only the last occurrence). I suggest using
"match any occurrence of pattern" to avoid confusion.
Oh, that would not indeed reflect the reality, but I think we need something
more verbose then, something along these lines (along with an example):
for SEARCH_FIRST:
"look for any occurrence of the argument with the expected value (this should
be used when an argument set to certain value overrides all the other
occurrences, e.g. when matching value 'on', arg='off arg='on'
arg='off' would
match with this flag)"
and for SEARCH_LAST:
"look for the last occurrence of argument with the expected value (this should
be used when the last occurrence of the argument overrides all the other ones,
e.g. when matching value 'on', arg='on' arg='off' would not match
with this
flag, but arg='off' arg='on' would)"
Does that sound better?
PS: sorry for the reverse diff I sent, wrong order when doing the diff :/
Erik
Everything else (including the changes in the other patches) looks good to
me.
> -/* 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, ¶m,
&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;
> }
>
>
--
Best regards,
Paulo de Rezende Pinatti