Re: [PATCH v3 2/3] x86/APIC: modify error_interrupt() to output using single printk()

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Thu, Jul 13, 2023 at 03:12:55PM +0200, Jan Beulich wrote:
> On 17.03.2023 20:53, Elliott Mitchell wrote:
> > --- a/xen/arch/x86/apic.c
> > +++ b/xen/arch/x86/apic.c
> > @@ -1419,12 +1420,13 @@ static void cf_check error_interrupt(struct 
> > cpu_user_regs *regs)
> >      v1 = apic_read(APIC_ESR);
> >      ack_APIC_irq();
> >  
> > -    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)",
> > -            smp_processor_id(), v , v1);
> >      for ( i = 7; i >= 0; --i )
> > -        if ( v1 & (1 << i) )
> > -            printk("%s", esr_fields[i]);
> > -    printk("\n");
> > +        entries[i] = v1 & (1 << i) ? esr_fields[i] : "";
> > +    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)"
> > +        "%s%s%s%s%s%s%s%s" "\n",
> > +        smp_processor_id(), v , v1,
> > +        entries[7], entries[6],
> > +        entries[5], entries[4], entries[3], entries[2], entries[1], 
> > entries[0]);
> 
> While pre-existing in both cases, two nits: There's a stray blank before one
> of the commas, and indentation is wrong too.

I don't see anything which could be called an indentation error.  The
very first added line is attached to the `for ()`, therefore it correctly
has one more indent.

I guess this is an advantage of rewriting the loop as part of this patch.
The correct indentation is clearer.

> Furthermore there's no reason to split the format string, especially not
> ahead of the \n. Plus line wrapping for the 8 entries[] references could
> also be done more evenly.
> 
> Since these are all cosmetic, I guess I'll do adjustments while committing:
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Yeah, that seems the best approach here.  I'm unsure how likely it is for
additional bits in the ESR register to be allocated.  Ideal is to format
this in such a way that adding bits causes a smaller delta.  I was
thinking high-order bits were likely to be reportted at start, hence the
last line would be best filled first.

Though one does not know the future and trying to anticipate it may well
yield worse results.  This really does seem a place where it should be
adjusted to taste during final commit.  Otherwise there are too many
minor nits and things get bogged down in trivial issues.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445