[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Fix for RSP vCont packet
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH] Fix for RSP vCont packet |
Date: |
Thu, 31 Jan 2019 06:58:12 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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 <address@hidden>
> ---
> 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
signature.asc
Description: OpenPGP digital signature