<URL: http://bugs.freeciv.org/Ticket/Display.html?id=40113 >

Madeline Book wrote:
> ... In the case where the client's player is then
> created (attach_connection_to_player(pconn, NULL) returns
> TRUE, server/connecthand.c +137) the server does not
> notify the client that its game.info.player_idx has
> changed (hence the client keeps its client.playing equal
> to NULL even though it should now have a player, and
> indeed receives conn and player info packets to that
> effect). ...
Good analysis.  This kind of inconsistency is always a problem whenever
there are multiple copies of the same data (in this case the player
pointer in several data structures).

> Solution:
> The server sends another game info packet after the
> client's player has been created. This ensures that
> the client receives an updated player_idx field and
> accordingly sets its client.playing to a valid pointer.
Ahem.  That's not exactly a solution, that's generally called a "hack" --
from the old days of applying a hacksaw to model railroads.

I've been trying to reduce *_info spam over the past 6 months, see PR#40000
and its predecessors.

A better (temporary) solution is to remove the validity checking, as the
player_idx is not always valid, despite the comments in the code.

I'm not a supporter of temporary solutions, when there are obvious
permanent solutions.  (See below.)

> .... Hence when running a forked server (started
> by the client using "Start New Game"), which sends
> hardcoded commands right upon connection (i.e. "/set
> aifill 5"), the client.playing variable in the client
> is made consistent (i.e. not NULL) before the user can
> notice any discrepancies.
Excellent catch!  This is how the problem was missed in the past.  Only by
pretending to be a second player in a multi-player game (when you are
actually the first), you bypassed the usual command processing.

Sending those hardcoded commands is a bad idea, too.  There are several
existing bug reports.

> It is strongly recommended that the game.info.player_idx
> field be completely removed as soon as convenient, and the
> client rely on a better designed and more logical mecha-
> nism to update its client.playing global variable (the
> conn info packet handler comes to mind).
I concur.  Almost all uses were removed in PR#39872.

However, a deeper analysis shows that game.info.player_idx is based upon:

     /* ? fixme: check for non-players: */
     ginfo.player_idx = (pconn->player ? player_number(pconn->player) : -1);

That is, a duplicate of the connection player!  Duplication be bad!

That fixme has been sitting there like a land-mine since before 1.14.2
(the earliest version that I have on hand).

As you suggest, the comprehensive solution is to replace player_idx with an
indication of the connection!  In turn, the connection has (or soon will
have) the player and observer status in it.

There is *no* need for the client.playing (nee game.player_ptr) variable.
This could also eliminate the duplicate aconnection.player (and other
duplicate data).

The connection number is learned during the login sequence.  It is never
changed or renumbered, unlike the player number.

Freeciv-dev mailing list

Reply via email to