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#4
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:
server/gamehand.c:370:
/* ? 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
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
