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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Jan,

Thank you for the feedback.

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.

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.

If empty tokens are intended to mean color 0, then I agree the second
patch should be dropped or reworked, and the syntax should be documented
explicitly instead. From the current in-code and user-facing docs,
rejecting them looked more consistent to me.

>
> In any event, you didn't Cc the authors of the patch, without which it
> seems unlikely that they might even notice the submission.

On Cc: I followed the submission workflow from the Xen Project
documentation, including the add_maintainers.pl.
The documentation says that, when following those steps, the scripts
will add the correct people automatically.

Apparently that was not sufficient in this case. I will add the
original authors explicitly if there is a next posting.

Best regards,
Mykola