Re: [Qemu-devel] [PATCH v9 15/27] gdbstub: Implement file io (F pkt) with new infra
Jon Doron 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 wrote:
>>
>>
>> Jon Doron 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
>> > ---
>> > 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,
>> > + _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 **), 16);
>> > -if (*p == ',') {
>> > -p++;
>> > -err = strtoull(p, (char **), 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 = _io_cmd_desc;
>> > }
>> > break;
>> > case 'g':
>>
>>
>> --
>> Alex Bennée
--
Alex Bennée
Re: [Qemu-devel] [PATCH v9 15/27] gdbstub: Implement file io (F pkt) with new infra
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 wrote:
>
>
> Jon Doron 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
> > ---
> > 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,
> > + _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 **), 16);
> > -if (*p == ',') {
> > -p++;
> > -err = strtoull(p, (char **), 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 = _io_cmd_desc;
> > }
> > break;
> > case 'g':
>
>
> --
> Alex Bennée
Re: [Qemu-devel] [PATCH v9 15/27] gdbstub: Implement file io (F pkt) with new infra
Jon Doron 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
> ---
> 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,
> + _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 **), 16);
> -if (*p == ',') {
> -p++;
> -err = strtoull(p, (char **), 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 = _io_cmd_desc;
> }
> break;
> case 'g':
--
Alex Bennée
