Re: [Qemu-devel] [PATCH] gdbstub: allow killing QEMU via vKill command

[Luc Michel]

Hi,

On 1/30/19 2:37 PM, KONRAD Frederic wrote:
> Hi,
> 
> Le 1/29/19 à 9:32 PM, Max Filippov a écrit :
>> With multiprocess extensions gdb uses 'vKill' packet instead of 'k' to
>> kill the inferior. Handle 'vKill' the same way 'k' was handled in the
>> presence of single process.
>>
>> Fixes: 7cf48f6752e5 ("gdbstub: add multiprocess support to
>> (f|s)ThreadInfo and ThreadExtraInfo")
Thanks for the patch.

>>
>> Cc: Luc Michel <address@hidden>
>> Signed-off-by: Max Filippov <address@hidden>
>> ---
>>   gdbstub.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index bfc7afb50968..1ef31240c055 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1383,6 +1383,28 @@ static int gdb_handle_packet(GDBState *s, const
>> char *line_buf)
>>                 put_packet(s, buf);
>>               break;
>> +        } else if (strncmp(p, "Kill;", 5) == 0) {
>> +            unsigned long pid;
>> +
>> +            p += 5;
>> +
>> +            if (qemu_strtoul(p, &p, 16, &pid)) {
>> +                put_packet(s, "E22");
>> +                break;
>> +            }
>> +            process = gdb_get_process(s, pid);
>> +
>> +            if (process == NULL) {
>> +                put_packet(s, "E22");
>> +                break;
>> +            }
>> +            if (s->process_num <= 1) {
>> +                /* Kill the target */
>> +                error_report("QEMU: Terminated via GDBstub");
>> +                exit(0);
> 
> I suggest:
> 
> #ifdef CONFIG_USER_ONLY
>         exit(0);
> #else
>         qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
You are missing a `break;' here.

Regarding the cause, I think it's more an 'host' cause than a 'guest' one.
> #endif
> 
> instead of exit(0);
I just tested, this is not ideal in the current shape. Not quitting
immediately makes the stub sends a "W00" packet. It's better if we send
nothing because GDB interprets that as the connection was closed (which
is effectively the case). Otherwise in multiprocess mode, GDB thinks the
other processes are still alive (see below).

with exit(0):
(gdb) k
Kill the program being debugged? (y or n) y
Sending packet: $vKill;1#6e...Ack
Remote connection closed

with qemu_system_shutdown_request:
(gdb) k
Kill the program being debugged? (y or n) y
Sending packet: $vKill;1#6e...Ack
Packet received: W00
Packet vKill (kill) is supported
[Inferior 1 (process 1) killed]

So maybe a solution (for another patch) would be to send a 'W' packet
for each attached process (using the multiprocess format). Another is to
prevent 'W' packets to be sent at all... they are not required in this case.

> 
>> +            }
>> +            /* TODO: handle multiprocess case */
About the multiprocess case, I would vote to adopt the same behaviour
(i.e. terminate QEMU), whatever the PID is or how many processes are
attached. Otherwise we would have different behaviour for the GDB `kill`
command, whether the simulated board exposes multiple processes or not.
Moreover, I think considering just one PID for a kill command does not
make much sense in our case.

I just tested, GDB is fine with QEMU closing the connection when having
multiple processes attached if we do exit(0) directly (i.e. do not send
a 'W00' packet).

Thanks.

Luc
>> +            goto unknown_command;
>>           } else {
>>               goto unknown_command;
>>           }
>>

signature.asc
Description: OpenPGP digital signature