Re: [PATCH] add file desc to gdbserver client_state

Tom Tromey
2020-01-24T18:12:21Z

>>>>> "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