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

[Jon Doron]

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.

-- 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