Re: [PATCH 0/2] xen/common: llc-coloring parser fixes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.05.2026 09:01, Mykola Kvach wrote:
> On Mon, May 18, 2026 at 9:52 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 18.05.2026 08:42, Mykola Kvach wrote:
>>> On Mon, May 18, 2026 at 9:20 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 16.05.2026 17:03, Mykola Kvach wrote:
>>>>> This small series fixes two issues in parse_color_config().
>>>>>
>>>>> The first patch makes parse failures leave the caller-visible color count
>>>>> at zero.  This prevents a rejected command-line value from leaving a
>>>>> partially parsed configuration behind for later init paths to consume.
>>>>>
>>>>> The second patch rejects empty color tokens.  Previously, delimiters in
>>>>> places where a color value was expected could be interpreted as color 0,
>>>>> because simple_strtoul() returns zero without advancing the input pointer.
>>>>> The patch checks that each parsed color value consumed input.  It also
>>>>> adds the missing newline to the DT color parsing error message.
>>>>>
>>>>> Mykola Kvach (2):
>>>>>   xen/common: llc-coloring: clear color count on parse failure
>>>>>   xen/common: llc-coloring: reject empty color tokens
>>>>
>>>> For both of these, a question which isn't even considered in the reasoning
>>>> is whether the present behavior may be intentional. Especially for the 2nd
>>>> ISTR Stefano(?) not so long ago indicating that the behavior is indeed
>>>> intended to be this way. That may have been somewhere on Matrix rather than
>>>> on the list, though.
>>>
>>> Thank you for pointing this out.
>>>
>>> For the first patch, my reasoning was that returning an error while
>>> leaving a partially parsed caller-visible color count behind looks
>>> surprising. If the value is rejected, I think later init paths should
>>> not be able to consume the partially parsed state.
>>>
>>> For the second patch, my reasoning was that the current behavior looks
>>> accidental rather than an intentional extension of the syntax.
>>
>> That was my impression as well, hence why I had raised the question back
>> then.
>>
>>> The parser comment says:
>>>
>>> COLOR_CONFIGURATION ::= COLOR | RANGE,...,COLOR | RANGE
>>> RANGE ::= COLOR-COLOR
>>>
>>> The user guide also describes this as a comma-separated list of colors
>>> or ranges, with ranges written as hyphen-separated inclusive intervals.
>>> I don't see an empty-token production there.
>>
>> What you quote is insufficient to determine: COLOR may be allowed to be
>> <nothing>. Iirc the reasoning went in particular towards a range with
>> merely the upper end specified being something (halfway) meaningful.
> 
> Right, I see your point.
> 
> The grammar does not define COLOR explicitly, so it does not by itself
> prove that an empty token is invalid. I was implicitly reading COLOR as
> a numeric color value, partly because all examples seem to use numeric
> values, but I agree that this is not stated there.
> 
> If this behavior is intentional, then I agree the second patch should
> not go in as-is.

Just to mention: Something like "-5" won't be interpreted as "0-5" even
right now. Instead it's taken as a single color with value -5U, afaict.

Jan