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

Paulo de Rezende Pinatti ppinatti at linux.ibm.com
Mon Jun 15 15:50:02 UTC 2020



On 15/06/20 17:46, Erik Skultety wrote:
> 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 at linux.ibm.com>
>>>> Signed-off-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
>>>
>>> Reviewed-by: Erik Skultety <eskultet at 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?

That sounds great :)

> 
> PS: sorry for the reverse diff I sent, wrong order when doing the diff :/

No worries :)

> 
> 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, &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;
>>>    }
>>>
>>>
>>
>> --
>> Best regards,
>>
>> Paulo de Rezende Pinatti
>>
> 

-- 
Best regards,

Paulo de Rezende Pinatti




More information about the libvir-list mailing list