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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.03.2023 16:54, Elliott Mitchell wrote:
> 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.

Just to clarify: What is or is not adjusted on commit is a decision of
the committer. A no longer as active committer was actually of the
opinion that it is a mistake to ever make any changes while committing.
In the case here you're asking for far more changes (including either
one to the description of patch 1, or the folding of both patches) than
I personally would be willing to do. I'm sorry for that.

Jan