Re: [PATCH] add file desc to gdbserver client_state

[Tom Tromey <tom@tromey.com>]

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

Stan> This patch adds gdb_fildes_t support to gdbserver for later use by the
Stan> gdbserver multi-client patch.  That patch enables multiple clients
Stan> with each client associated with its corresponding file desc.   This
Stan> patch adds the corresponding file desc to the client_state and adds a
Stan> file desc parameter to the gdbserver I/O ops.  It also adds a minimal
Stan> multi_client_states which currently just manages a single client_state
Stan> but later will be expanded to be a collection of multiple
Stan> client_states where each client_state is the state for an individual
Stan> client.  The gdbserver multi-client patch is mentioned in:
Stan> https://sourceware.org/ml/gdb-patches/2019-08/msg00577.html

Thanks.

This looks like a reasonable first step.  Hopefully a few things will
change in coming patches?  I'll note some below.  Also, I think in
general it needs a few more comments and some formatting cleanup.


Stan> +
Stan> +int
Stan> +get_remote_desc (void)
Stan> +{

In gdb new functions need a comment -- usually a description in the
header and then something like "see mumble.h" at the implementation.

Will remote_desc be going away?  And hopefully get_remote_desc is also
just a stepping stone?

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

Should be "&client_states", without a space.

Stan> +multi_client_states&

"multi_client_states &".

Stan> +get_client_states (void)

No more "(void)" now that we're using C++.  I think there's a few spots
for this.

Stan> +client_state&

"client_state &".

Stan> +multi_client_states::set_client_state (gdb_fildes_t fd)
Stan> +{
Stan> +  client_state &cs = get_client_state ();

Can get_client_state be made static now?
If not, why not?

Stan> +  int remote_desc = get_remote_desc();

"get_remote_desc ()".  But why does this use int while other spots use
gdb_fildes_t?

Stan> --- a/gdb/gdbserver/server.h
Stan> +++ b/gdb/gdbserver/server.h
Stan> @@ -142,6 +142,8 @@ struct client_state
Stan>      own_buf ((char *) xmalloc (PBUFSIZ + 1))
Stan>    {}

Stan> +  gdb_fildes_t file_desc;

Needs a comment.


Stan> -client_state &get_client_state ();
Stan> +struct multi_client_states
Stan> +{
Stan> +public:

Needs a comment.
Probably should say "class" and not "struct", particularly since it's
using "public:".

Stan> +  client_state & set_client_state (gdb_fildes_t);

"&set_client_state".  Needs a comment.

Stan> +client_state & get_client_state ();
Stan> +struct multi_client_states & get_client_states (void);

Extra spaces and lack of comments here too.

Tom