| Message ID | 062ed51e7529d282b6e336c8b62734afaf21979f.1689191941.git.ehem+xen@m5p.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixing ACPI error reporting display | expand |
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@m5p.com> Acked-by: Jan Beulich <jbeulich@suse.com> Nevertheless I wonder ... > --- a/xen/arch/x86/apic.c > +++ b/xen/arch/x86/apic.c > @@ -1401,14 +1401,14 @@ static void cf_check spurious_interrupt(struct cpu_user_regs *regs) > static void cf_check error_interrupt(struct cpu_user_regs *regs) > { > static const char *const esr_fields[] = { > - "Send CS error", > - "Receive CS error", > - "Send accept error", > - "Receive accept error", > - "Redirectable IPI", > - "Send illegal vector", > - "Received illegal vector", > - "Illegal register address", > + ", Send CS error", > + ", Receive CS error", > + ", Send accept error", > + ", Receive accept error", > + ", Redirectable IPI", > + ", Send illegal vector", > + ", Received illegal vector", > + ", Illegal register address", > }; > unsigned int v, v1; > int i; > @@ -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 ... Jan
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@m5p.com> > > Acked-by: Jan Beulich <jbeulich@suse.com> > > 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().
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c index f71474d47d..8cfb8cd71c 100644 --- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -1401,14 +1401,14 @@ static void cf_check spurious_interrupt(struct cpu_user_regs *regs) static void cf_check error_interrupt(struct cpu_user_regs *regs) { static const char *const esr_fields[] = { - "Send CS error", - "Receive CS error", - "Send accept error", - "Receive accept error", - "Redirectable IPI", - "Send illegal vector", - "Received illegal vector", - "Illegal register address", + ", Send CS error", + ", Receive CS error", + ", Send accept error", + ", Receive accept error", + ", Redirectable IPI", + ", Send illegal vector", + ", Received illegal vector", + ", Illegal register address", }; unsigned int v, v1; int i; @@ -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]); printk("\n"); }
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@m5p.com> --- v2: One more sentence in the commit message. --- xen/arch/x86/apic.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)