Re: [PATCH 10/10] remote: one struct remote_state per struct remote_target

[Simon Marchi <simark at simark dot ca>]

On 2018-05-16 10:18 AM, Pedro Alves wrote:
> 'struct remote_state' today contains per-connection state, however
> there's only a single global instance of that type.  In order to
> support multiple connections, we must have one such object per
> connection.
> 
> Thus this patch eliminates the 'remote_state' global in favor of
> having a remote_state instance per remote_target instance.
> 
> The get_remote_state free function is eliminated as well, by making it
> a remote_target method instead.
> 
> The patch then fixes the fallout by making all free functions that
> refer to get_remote_state() directly or indirectly be methods of
> remote_target too.
> 
> Likewise, remote-fileio.c and remote-notif.c routines are
> parameterized with a remote_target pointer too, so they can call into
> the right remote_target instance.
> 
> References to the global 'get_remote_state ()->remote_desc' to tell
> whether the remote target is open (!= nullptr) must be replaced with
> something else:
> 
>  - Command implementations use a new get_current_remote_target free
>  function.
> 
>  - remote_target::open_1 checks the exception type instead.
> 
> Finally, remote_target and extended_remote_target are made
> heap-allocated targets.  As with the earlier core target patches, it
> still won't be possible to have more than one remote_target instance
> in practice, but this puts us closer.

Hi Pedro,

I took a rather quick look, because a lot of the changes are mecanical,
once you have set the premises you pointed out in the commit message
(and I think they are fine).

Two nits:

Is there a reason not to make the remote_state object a simple field
of remote_target, does it have to be a pointer?  You would have to
shuffle things around a little bit more, but it seems to work fine.

> @@ -6287,7 +6512,7 @@ remote_target::commit_resume ()
>       we end up with too many actions for a single packet vcont_builder
>       flushes the current vCont packet to the remote side and starts a
>       new one.  */
> -  struct vcont_builder vcont_builder;
> +  struct vcont_builder vcont_builder (this);
>    vcont_builder.restart ();

That's more a comment for the previous patch, but: I find it strange to
have to call restart just after building the object. Couldn't the
constructor leave it in a ready to use state?

Simon