Re: [Qemu-devel] [PATCH v9 15/27] gdbstub: Implement file io (F pkt) with new infra

[Alex Bennée]

Jon Doron <address@hidden> writes:

> Hi Alex, I did have some issues with the 'F' packet as it's not really
> well documented, I suggest changing the schema to:
> "L,L,o0"
> so basically no support for anything after the first C in the Ctrl-C,
> if you have a sample or a documentation that really implements
> the F packet fully ill take a look at it and see how the schema should
> really look like.

I'm only going by whats in the gdbdocs:

  
https://sourceware.org/gdb/onlinedocs/gdb/The-F-Request-Packet.html#The-F-Request-Packet

>
> -- Jon.
>
> On Wed, May 15, 2019 at 7:54 PM Alex Bennée <address@hidden> wrote:
>>
>>
>> Jon Doron <address@hidden> writes:
>>
>> There is a bit more going on here than a simple conversion. I think we
>> need some additional commentary about the format of the data coming
>> back.
>>
>>
>> > Signed-off-by: Jon Doron <address@hidden>
>> > ---
>> >  gdbstub.c | 62 +++++++++++++++++++++++++++++++++++--------------------
>> >  1 file changed, 40 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/gdbstub.c b/gdbstub.c
>> > index 3478ac778d..9fe130f30d 100644
>> > --- a/gdbstub.c
>> > +++ b/gdbstub.c
>> > @@ -1772,6 +1772,39 @@ static void handle_read_all_regs(GdbCmdContext 
>> > *gdb_ctx, void *user_ctx)
>> >      put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>> >  }
>> >
>> > +static void handle_file_io(GdbCmdContext *gdb_ctx, void *user_ctx)
>> > +{
>> > +    int num_syscall_params;
>> > +    GdbCmdVariant syscall_params[3] = {};
>> > +
>> > +    if (!gdb_ctx->num_params) {
>> > +        return;
>> > +    }
>> > +
>> > +    if (cmd_parse_params(gdb_ctx->params[0].data, "L,L,o0", 
>> > syscall_params,
>> > +                         &num_syscall_params)) {
>> > +        return;
>> > +    }
>>
>> What's going on here? I thought the schema was meant to handle the
>> parsing of data. I see bellow we originally parse the command as a null
>> terminated string but we actually should handle:
>>
>>   ‘Fretcode,errno,Ctrl-C flag;call-specific attachment’
>>
>> I see the argument for dealing with the call-specific attachment here
>> but shouldn't the generic parsing code be able to split everything
>> apart?
>>
>> > +
>> > +    if (!num_syscall_params) {
>> > +        return;
>> > +    }
>> > +
>> > +    if (gdb_ctx->s->current_syscall_cb) {
>> > +        gdb_ctx->s->current_syscall_cb(gdb_ctx->s->c_cpu,
>> > +                                       
>> > (target_ulong)syscall_params[0].val_ull,
>> > +                                       
>> > (target_ulong)syscall_params[1].val_ull);
>> > +        gdb_ctx->s->current_syscall_cb = NULL;
>> > +    }
>>
>>
>>
>> > +
>> > +    if (syscall_params[2].opcode == (uint8_t)'C') {
>> > +        put_packet(gdb_ctx->s, "T02");
>> > +        return;
>> > +    }
>> > +
>> > +    gdb_continue(gdb_ctx->s);
>> > +}
>> > +
>> >  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> >  {
>> >      CPUState *cpu;
>> > @@ -1913,28 +1946,13 @@ static int gdb_handle_packet(GDBState *s, const 
>> > char *line_buf)
>> >          return RS_IDLE;
>> >      case 'F':
>> >          {
>> > -            target_ulong ret;
>> > -            target_ulong err;
>> > -
>> > -            ret = strtoull(p, (char **)&p, 16);
>> > -            if (*p == ',') {
>> > -                p++;
>> > -                err = strtoull(p, (char **)&p, 16);
>> > -            } else {
>> > -                err = 0;
>> > -            }
>> > -            if (*p == ',')
>> > -                p++;
>> > -            type = *p;
>> > -            if (s->current_syscall_cb) {
>> > -                s->current_syscall_cb(s->c_cpu, ret, err);
>> > -                s->current_syscall_cb = NULL;
>> > -            }
>> > -            if (type == 'C') {
>> > -                put_packet(s, "T02");
>> > -            } else {
>> > -                gdb_continue(s);
>> > -            }
>> > +            static const GdbCmdParseEntry file_io_cmd_desc = {
>> > +                .handler = handle_file_io,
>> > +                .cmd = "F",
>> > +                .cmd_startswith = 1,
>> > +                .schema = "s0"
>> > +            };
>> > +            cmd_parser = &file_io_cmd_desc;
>> >          }
>> >          break;
>> >      case 'g':
>>
>>
>> --
>> Alex Bennée


--
Alex Bennée