Re: [PATCH] add file desc to gdbserver client_state

[Tom Tromey <tom@tromey.com>]

>>>>> "Stan" == Stan Cox <scox@redhat.com> writes:

Stan>     Add struct multi_client_states
    
Stan>     * remote-utils.c (remote_desc):  Move to struct client_state
Stan>       (gdb_connected, handle_accept_event, remote_open, putpkt_binary_1)
Stan>       (putpkt_notif, input_interrupt, block_unblock_async_io)
Stan>       (nto_conctrl, getpkt): Change remote_desc reference.
Stan>     * server.c (captured_main): Move remote_desc initialization to
Stan>       remote_prepare.

Thanks for the patch.

Stan> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
Stan> index d7da4b7aed..09095d9bbc 100644
Stan> --- a/gdb/gdbserver/remote-utils.c
Stan> +++ b/gdb/gdbserver/remote-utils.c
Stan> @@ -25,6 +25,7 @@
Stan>  #include "tdesc.h"
Stan>  #include "debug.h"
Stan>  #include "dll.h"
Stan> +#include "server.h"

I think this is already included near the top.
Normally, .c files in gdbserver include this first.

Stan> +
Stan>  int
Stan>  gdb_connected (void)

Spurious newline addition.

Stan> +  struct multi_client_states &client_states = get_client_states();

Needs a space before the "(".

Stan> +  get_client_states().set_current_client (cs);

Ditto.

Stan> +/* Container of client remote protocol states for all the currently
Stan> +   connected clients.  */
Stan> +
Stan> +#define get_client_state() get_client_states().get_current_client()
Stan> +
Stan> +class multi_client_states
Stan> +{
Stan> +private:
Stan> +  client_state *current_cs;
Stan> +
Stan> +public:
Stan> +  /* Return the current client we are focused on. */
Stan> +  client_state &get_current_client () { return *current_cs; }
Stan> +
Stan> +  /* Set the current client we wish to focus on. */
Stan> +  void set_current_client (client_state &cs) { current_cs = &cs; }

I think this class is a bit strange.
It stores a pointer but the set_ method takes a reference.

If this class is going to own multiple clients in the future, then I
think it makes sense to design the API that way from the start.

thanks,
Tom