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

Simon Marchi
2018-05-22T03:37:35Z

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