[Freeciv-Dev] Re: (PR#3521) Re: (PR#3601) Intelligence dialog empty

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Mon, Mar 03, 2003 at 07:18:53PM -0800, Jason Short wrote:
> Per I. Mathisen wrote:
> > On Tue, 4 Mar 2003, Per I. Mathisen wrote:
> > 
> >>On Mon, 3 Mar 2003, Per I. Mathisen wrote:
> >>
> >>>This fixes the symptom, and I think it can suffice for now:
> >>
> >>Well, here is another and better fix. Attached. Kudos to Vasco for coming
> >>up with the solution.
> > 
> > 
> > A third fix, kudos to Jason. It also works.
> > 
> > Index: client/packhand.c
> > ===================================================================
> > RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
> > retrieving revision 1.293
> > diff -u -r1.293 packhand.c
> > --- client/packhand.c   2003/02/20 23:00:55     1.293
> > +++ client/packhand.c   2003/03/04 02:30:33
> > @@ -1309,7 +1309,7 @@
> > 
> >    pplayer->is_connected = pinfo->is_connected;
> > 
> > -  if (pplayer->is_connected) {
> > +  if (game.num_tech_types > 0) {
> >      read_player_info_techs(pplayer, pinfo->inventions);
> >    }
> > 
> > Raimar, now you have lots of fixes to choose between!

> After a little bit of thought, I think that initializing the data in 
> player.c is a hack.

Yes.

> The thing is that only the first 
> game.num_tech_types technologies are (supposed to be) guaranteed to have 
> valid data, so to assert that the first (A_NONE) tech is TECH_KNOWN 
> simply doesn't make sense if game.num_tech_types==0.  

> Moreover, spurious initialization may conceal other errors (like
> accessing invalid data) that could otherwise be more easily caught
> (by valgrind, for instance).

Yes.

> The above patch is one possible fix based on that idea.  It's also 
> possible to put the check into read_player_info_techs, or to push it all 
> the way into update_research and just change the assert to
>    assert(game.num_tech_types == 0 || get_tech(...) == TECH_KNOWN));

> But, none of this actually works because there *is* an actual bug here. 
>   If you start a game with two human players, then the technologies will 
> be read and every player's techs initialized (I think).  But if a player 
> then disconnects, the player data will be sent out again without ever 
> being (correctly) initialized.  In this case we'll still get an 
> assertion under the above patch.  This is most likely harmless, because 
> immediately after this the game restarts.  It does get fixed by 
> initializing the data in player.c, but this still seems wrong to me.

Yes. The problem: init_tech (which marks all techs given in the
ruleset unknown, makes A_NONE and gives init techs) is only called in
a new game and if all players chose their nation. So this leaves the
time window from the time a player connects and this is published to
all other players till the time this player disconnects before the
init_tech is called. In this time window the tech data is unset. This
is the problem that the previous patch should have fixed.

Solution 1 to the current problem is to initialize the data before
this time window and remove the check for a connected player. This is
IMHO bad since it pretends that there is valid data if there isn't
any.

Solution 2 is to correct the check to catch the above time
window. The attached patch tries this. The patch is untested.

> Note valgrind does not report any uninitialized memory errors for 
> civserver.  I guess all the player data is zeroed out, so if we do 
> initialize it in player.c the memset is unnecessary and only A_NONE 
> needs to be set.

game is a global variable. See http://www.eskimo.com/~scs/C-faq/q1.30.html.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "SIGDANGER - The System is likely to crash soon"

Index: client/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.293
diff -u -u -r1.293 packhand.c
--- client/packhand.c   2003/02/20 23:00:55     1.293
+++ client/packhand.c   2003/03/04 11:18:11
@@ -1309,7 +1309,9 @@
 
   pplayer->is_connected = pinfo->is_connected;
 
-  if (pplayer->is_connected) {
+  if (pplayer->is_connected
+      || get_client_state() == CLIENT_GAME_RUNNING_STATE
+      || get_client_state() == CLIENT_GAME_OVER_STATE) {
     read_player_info_techs(pplayer, pinfo->inventions);
   }