Re: [PATCH v3 1/3] x86/APIC: include full string with error_interrupt() error messages

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Thu, Jul 13, 2023 at 03:08:56PM +0200, Jan Beulich wrote:
> On 17.03.2023 20:45, Elliott Mitchell wrote:
> > Rather than adding ", " with each printf(), simply include them in the
> > string initially.  This allows converting to strlcat() or other methods
> > which strictly concatenate, rather than formatting.
> > 
> > Signed-off-by: Elliott Mitchell <ehem+xen@xxxxxxx>
> 
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Nevertheless I wonder ...


> > @@ -1423,7 +1423,7 @@ static void cf_check error_interrupt(struct 
> > cpu_user_regs *regs)
> >              smp_processor_id(), v , v1);
> >      for ( i = 7; i >= 0; --i )
> >          if ( v1 & (1 << i) )
> > -            printk(", %s", esr_fields[i]);
> > +            printk("%s", esr_fields[i]);
> 
> ... whether the extra level of indirection (by using %s) is then still
> necessary: There are no % characters in any of the individual strings.
> Then again iirc this goes away anyway in the next patch ...

I suspect most development guidelines generally discourage this.  Too
easy for someone to later add a '%' to the string, or make it a dynamic
string coming from some random location.

More notable is that last point.  Emphasis here is the ", " being merged
into the strings and the line is being completely replaced in the next
patch.  v0 was to strcpy() into a buffer, but this is simpler.

I also suspect "%s" may be faster if the string is sufficiently long,
as this amounts to a strcpy() info printk's buffer.  Whereas avoiding the
indirection causes the string to be scanned for '%' which will be slower
than strcpy().


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