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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Mon, May 18, 2026 at 10:43 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 18.05.2026 09:19, Mykola Kvach wrote:
> > On Mon, May 18, 2026 at 10:06 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> 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.
> >
> > I don't think that is what happens with the current parser.
> >
> > I tested this without the patches from this series:
> >
> > (XEN) Command line: dom0_mem=2048M console=dtuart dtuart=serial0
> > (XEN) loglvl=all console_timestamps=boot llc-coloring=on
> > (XEN) xen-llc-colors=-5
> > ...
> > (XEN) LLC coloring info:
> > (XEN) Number of LLC colors supported: 32
> > (XEN) Xen LLC colors (6): { 0-5 }
> >
> > So "-5" is currently interpreted as "0-5", not as a single color with
> > value -5U.
> >
> > That seems to happen because simple_strtoul() does not consume the
> > leading '-', so start remains 0 and the parser then takes the range
> > path.
>
> Oh, I didn't recall this incompatibility with strtoul(). We also don't
> permit a leading + there. IOW the behavior of the color parsing would
> change if we made the functions (more) compatible with the standard.

Right.

The patch makes this explicit in one direction: a COLOR must consume
input. If "-N" is intended to be valid syntax, then I think it should be
handled as a separate explicit case, rather than relying on
simple_strtoul() stopping at '-'.

Best regards,
Mykola