Re: [PATCH] Discard events for a given remote_state from stop_reply_queue

[Pedro Alves <palves at redhat dot com>]

On 10/06/2013 08:04 AM, Yao Qi wrote:
> Nowadays, when a remote connection is closed, GDB will discard
> all stop replies in stop_reply_queue, which is not right if we
> have multiple connections.
> 
> This patch teaches GDB to discard events for a given remote
> connection (represented by remote_state), from stop_reply_queue.
> 
> After this patch, the remote notification should work well in the
> multi-connection case.

Thanks.  Looks good.  Comments on some of the comments below.

> ---
>  gdb/remote.c |   39 +++++++++++++++++++++++++++++++++------
>  1 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 60086d2..12248cc 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -220,7 +220,7 @@ struct stop_reply;
>  static void stop_reply_xfree (struct stop_reply *);
>  static void remote_parse_stop_reply (char *, struct stop_reply *);
>  static void push_stop_reply (struct stop_reply *);
> -static void discard_pending_stop_replies_in_queue (void);
> +static void discard_pending_stop_replies_in_queue (struct remote_state *);
>  static int peek_stop_reply (ptid_t ptid);
>  
>  static void remote_async_inferior_event_handler (gdb_client_data);
> @@ -3069,7 +3069,7 @@ remote_close (void)
>  
>    /* We are closing the remote target, so we should discard
>       everything of this target.  */
> -  discard_pending_stop_replies_in_queue ();
> +  discard_pending_stop_replies_in_queue (rs);
>  
>    if (remote_async_inferior_event_token)
>      delete_async_event_handler (&remote_async_inferior_event_token);
> @@ -5274,6 +5274,11 @@ typedef struct stop_reply
>    /* The identifier of the thread about this event  */
>    ptid_t ptid;
>  
> +  /* The remote state this event associated with.  When the remote

The remote state this event IS associated with.

> +     connection, represented by a remote_state object, is closed,
> +     all the stop_reply events should be released.  */

Feels like a word is missing here too.  I'd suggest one of:

     all the CORRESPONDING stop_reply events should be released.
     all the ASSOCIATED stop_reply events should be released.

> +  struct remote_state *rs;
> +
>    struct target_waitstatus ws;
>  
>    /* Expedited registers.  This makes remote debugging a bit more
> @@ -5434,19 +5439,40 @@ discard_pending_stop_replies (struct inferior *inf)
>  		 remove_stop_reply_for_inferior, &param);
>  }
>  
> -/* Discard the stop replies in stop_reply_queue.  */
> +/* Remove stop replies in the queue if its remote state is equal to
> +   the given remote state.  */

"remove ... from the queue"

"stop replies" is plural, so "if their".  But I think you
really wanted singular, so "stop reply" instead.

/* Remove stop reply from the queue if its remote state is equal
   to the given remote state.  */

It may not be crystal clear to the reader whether "its" is
referring to the stop reply, or the queue.  I'd suggest swapping
things a little:

/* If its remote state is equal to the given remote state,
   remove EVENT from the stop reply queue.  */

> +
> +static int
> +remove_stop_reply_for_remote_state (QUEUE (stop_reply_p) *q,

I'd s/for/of/.

> +				    QUEUE_ITER (stop_reply_p) *iter,
> +				    stop_reply_p event,
> +				    void *data)

-- 
Pedro Alves