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

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Mon, Mar 20, 2023 at 04:39:48PM +0100, Jan Beulich wrote:
> On 20.03.2023 15:29, Elliott Mitchell wrote:
> > 
> > There are several minor issues here which may be best handled during
> > commit as they're very small items about how precisely you want this to
> > look.
> > 
> > First, I later realized I goofed the argument order.  In order to match
> > the original implementation, it needs to be entries[7] ... entries[0]
> > (could though be the low-order bits should be reported first).
> 
> I'm not really concerned of the order. A change of order wants
> mentioning in the description though.

Seemed simple enough to fix on commit (simply switch the order of
numbers).

> > Second, the order of the for loop no longer matters.  Using
> > ARRAY_SIZE(esr_fields) and increment should now be more maintainable
> > (this would also allow i to be unsigned).
> 
> Indeed. But that would better done in a separate patch then anyway.

Feel free to split.

> > Third, I'm simply unsure how you would prefer to format the printk().
> 
> About any way matching style guidelines is okay. There are two more
> things to mention though (sorry for not noticing earlier): We aim at
> keeping the entire format string on one line, for grep-ability. And
> there's no need (and in fact no reason) to split the sequence of %s
> from the \n. To summarize:
> 
>     printk(XENLOG_DEBUG
>            "APIC error on CPU%u: %02x(%02x)%s%s%s%s%s%s%s%s\n",
> 
> (unless of course it all fits on one line, which it looks like it
> does).

I like keeping the "%s%s%s%s%s%s%s%s" section separated since it needs to
match the number of arguments.  In the future where more bits of the
register are defined, both sections will need to be modified together.


This seems to be a spot where there are large numbers of similarly
functional, but mildly different style variants.  As such I suspect this
is best left in your hands as this is a bog of trivial style
considerations which have no real functional effect.


-- 
(\___(\___(\______          --=> 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