Re: [PATCH 2/2] xen/common: llc-coloring: reject empty color tokens

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

Thank you for the review.

On Mon, May 18, 2026 at 9:40 AM <dmukhin@xxxxxxxx> wrote:
>
> On Sat, May 16, 2026 at 06:03:12PM +0300, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > parse_color_config() currently accepts delimiters where a color value
> > is expected because simple_strtoul() returns zero without advancing the
> > input pointer. This makes strings such as ",2-6", "-10,19-20" or
> > "1,,2" look as if an empty value was color 0.
> >
> > Also add the missing newline to the DT color parsing error message.
> >
> > Fixes: 6cdea3444eaf ("xen/arm: add Dom0 cache coloring support")
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> >  xen/common/llc-coloring.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/common/llc-coloring.c b/xen/common/llc-coloring.c
> > index 2606cb0977..5d00d4b40e 100644
> > --- a/xen/common/llc-coloring.c
> > +++ b/xen/common/llc-coloring.c
> > @@ -64,14 +64,21 @@ static int __init parse_color_config(const char *buf, 
> > unsigned int colors[],
> >
> >      while ( *s != '\0' )
> >      {
> > +        const char *endp;
> >          unsigned int color, start, end;
> >
> > -        start = simple_strtoul(s, &s, 0);
> > +        start = simple_strtoul(s, &endp, 0);
> > +        if ( endp == s )
> > +            goto fail;
> > +        s = endp;
> >
> >          if ( *s == '-' )    /* Range */
> >          {
> >              s++;
> > -            end = simple_strtoul(s, &s, 0);
> > +            end = simple_strtoul(s, &endp, 0);
> > +            if ( endp == s )
> > +                goto fail;
> > +            s = endp;
> >          }
> >          else                /* Single value */
> >              end = start;
> > @@ -334,7 +341,7 @@ int __init domain_set_llc_colors_from_str(struct domain 
> > *d, const char *str)
> >      err = parse_color_config(str, colors, max_nr_colors, &num_colors);
> >      if ( err )
> >      {
> > -        printk(XENLOG_ERR "Error parsing LLC color configuration");
> > +        printk(XENLOG_ERR "Error parsing LLC color configuration\n");
>
> While here, add domain ID to the printout similarly to
> `if ( !check_colors(..) )` processing below?

Yes, that makes sense.

If this patch goes forward, I will add the domain ID to this error
message in the next version.

Best regards,
Mykola