Re: [Qemu-devel] [PATCH] Fix for RSP vCont packet

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3504 bytes --]

On 1/30/19 10:48 PM, Lucien Murray-Pitts wrote:
> The result is that vCont now does not recognise the case where no process/thread is provided after the action.

Thanks for re-sending; this one came through a lot nicer!  Welcome to
the community; while a first submission can always be a bit daunting,
the maintainers generally will help fix things up as you refine your
process for making future submissions run more smoothly.

My next suggestion: wrap your commit message body around 65-70 columns,
rather than using long lines. While it may sound archaic, many
developers still use 80-column terminals, and 'git log' adds
indentation; long lines become difficult to read if you don't manually
wrap them.

> 
> This may not show up with GDB, but using Lauterbach Trace32, and Hexrays IDA Pro this issue is immediately seen.
> The response is a "$#00" empty packet, showing it is unsupported packet.
> 
> This is defined in the RSP document as "An action with no thread-id matches all threads."
> (https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#vCont-packet )
> 
> Thus the valid vCont packets now are as below, however parsing is still not very strict.
>   vCont;c/s                 - Step/Continue all threads
>   vCont;c/s:[pX.]Y          - Step/Continue optional process X, thread Y
>   vCont;C##/S##:[pX.]Y      - Step/Continue with signal ## on optional process X, thread Y
>   * If X or Y are -1 then it applies the action to all processes/threads.
> 
> Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com>
> ---
>  gdbstub.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)

I'll let someone more familiar with gdbstub review the actual patch
content for correctness, but I'll do a quick pass for style:

> 
> diff --git a/gdbstub.c b/gdbstub.c
> index bfc7afb509..ce0dde2e24 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1169,6 +1169,7 @@ static int is_query_packet(const char *p, const char *query, char separator)
>   */
>  static int gdb_handle_vcont(GDBState *s, const char *p)
>  {
> +    GDBThreadIdKind vcontThreadType ;

No space before ;

>      int res, signal = 0;
>      char cur_action;
>      char *newstates;
> @@ -1218,12 +1219,23 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>              goto out;
>          }
>  
> -        if (*p++ != ':') {
> +        /*
> +         * In the case we have vCont;c or vCont;s - action is on all threads

Grammar suggestion: s/In the case/When/, and use a trailing '.' to end
the sentence.

> +         * Alternatively vCont;c;s:p1.1 is a possible, but meaningless format,
> +         * And in the else the "vCont;c:p1.1;... format is supported.

s/And in the else/otherwise/

> +         */
> +        if (*p == '\0' || *p == ';') {
> +            vcontThreadType = GDB_ALL_THREADS ;
> +            pid = 1 ;
> +            tid = 1 ;

Again, no space before ;

> +        } else if (*p++ == ':') {
> +            vcontThreadType = read_thread_id(p, &p, &pid, &tid) ;
> +        } else {
>              res = -ENOTSUP;
>              goto out;
>          }
>  
> -        switch (read_thread_id(p, &p, &pid, &tid)) {
> +        switch (vcontThreadType) {
>          case GDB_READ_THREAD_ERR:
>              res = -EINVAL;
>              goto out;
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]