Re: [PATCH v2 09/53] target/avr: convert to use format_state instead of dump_state

[Philippe Mathieu-Daudé]

On 9/15/21 10:49 AM, Daniel P. Berrangé wrote:
> On Wed, Sep 15, 2021 at 09:13:14AM +0200, Philippe Mathieu-Daudé wrote:
>> On 9/14/21 4:19 PM, Daniel P. Berrangé wrote:
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>  target/avr/cpu.c | 57 ++++++++++++++++++++++++------------------------
>>>  1 file changed, 29 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
>>> index ea14175ca5..17ff21f8be 100644
>>> --- a/target/avr/cpu.c
>>> +++ b/target/avr/cpu.c
>>> @@ -145,43 +145,44 @@ static ObjectClass *avr_cpu_class_by_name(const char 
>>> *cpu_model)
>>>      return oc;
>>>  }
>>>  
>>> -static void avr_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>>> +static void avr_cpu_format_state(CPUState *cs, GString *buf, int flags)
>>>  {
>>>      AVRCPU *cpu = AVR_CPU(cs);
>>>      CPUAVRState *env = &cpu->env;
>>>      int i;
>>>  
>>> -    qemu_fprintf(f, "\n");
>>> -    qemu_fprintf(f, "PC:    %06x\n", env->pc_w * 2); /* PC points to words 
>>> */
>>> -    qemu_fprintf(f, "SP:      %04x\n", env->sp);
>>> -    qemu_fprintf(f, "rampD:     %02x\n", env->rampD >> 16);
>>> -    qemu_fprintf(f, "rampX:     %02x\n", env->rampX >> 16);
>>> -    qemu_fprintf(f, "rampY:     %02x\n", env->rampY >> 16);
>>> -    qemu_fprintf(f, "rampZ:     %02x\n", env->rampZ >> 16);
>>> -    qemu_fprintf(f, "EIND:      %02x\n", env->eind >> 16);
>>> -    qemu_fprintf(f, "X:       %02x%02x\n", env->r[27], env->r[26]);
>>> -    qemu_fprintf(f, "Y:       %02x%02x\n", env->r[29], env->r[28]);
>>> -    qemu_fprintf(f, "Z:       %02x%02x\n", env->r[31], env->r[30]);
>>> -    qemu_fprintf(f, "SREG:    [ %c %c %c %c %c %c %c %c ]\n",
>>> -                 env->sregI ? 'I' : '-',
>>> -                 env->sregT ? 'T' : '-',
>>> -                 env->sregH ? 'H' : '-',
>>> -                 env->sregS ? 'S' : '-',
>>> -                 env->sregV ? 'V' : '-',
>>> -                 env->sregN ? '-' : 'N', /* Zf has negative logic */
>>> -                 env->sregZ ? 'Z' : '-',
>>> -                 env->sregC ? 'I' : '-');
>>> -    qemu_fprintf(f, "SKIP:    %02x\n", env->skip);
>>> -
>>> -    qemu_fprintf(f, "\n");
>>> +    g_string_append_printf(buf, "\n");
>>
>> This would be g_string_append_c(buf, '\n') but in this particular case
>> it doesn't seem helpful so I'd directly remove it.
> 
> I don't want to change output format of the commands, with exception of
> error reporting, as this is intended to be just refactoring patch, not
> a cleanup patch.

I misread cpu_dump_state(), I thought it was already appending
a newline, but it is not:

@@ -106,6 +106,21 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
     if (cc->dump_state) {
         cpu_synchronize_state(cpu);
         cc->dump_state(cpu, f, flags);
+    } else if (cc->format_state) {
+        g_autoptr(GString) buf = g_string_new("");
+        cpu_synchronize_state(cpu);
+        cc->format_state(cpu, buf, flags);
+        qemu_fprintf(f, "%s", buf->str);

So we are fine.
> I'm not convinced it is worth special casing single byte strings to
> use g_string_append_c either really.

OK. R-b stands ;)