Complete.Org: Mailing Lists: Archives: freeciv-dev: January 2002:
[Freeciv-Dev] [PATCH] bug in vnotify_conn_ex() (PR#1185)
Home

[Freeciv-Dev] [PATCH] bug in vnotify_conn_ex() (PR#1185)

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: freeciv-dev@xxxxxxxxxxx
Cc: bugs@xxxxxxxxxxxxxxxxxxx
Subject: [Freeciv-Dev] [PATCH] bug in vnotify_conn_ex() (PR#1185)
From: jdorje@xxxxxxxxxxxxxxxxxxxxx
Date: Wed, 2 Jan 2002 00:47:01 -0800 (PST)

This buglet is very similar to PR#1184.

The function vnotify_conn_ex() notifies a player of an event. A coordinate set can optionally be passed in to give the location of the event, or (-1,-1) can be passed to identify a "no-location" event. This is a small hack, but nothing much to worry about.

There are two problems with the internals of vnotify_conn_ex(), though. Both deal with this line:

    if (is_real_tile(x, y) && server_state >= RUN_GAME_STATE

1. Calling is_real_tile before the game starts is generally no good. Under general-topologies, it will result in a failed assertion as the topology will not yet have been initialized. Thus it should be clear that the order here should be reversed.

2. Again, is_real_tile may return true for (-1,-1) under an alternate topology. The easy solution here is just to change it to is_normal_map_pos. There are still some minor pitfalls: (1) it obviously won't work if is_normal_map_pos(-1,-1), and (2) we need to be careful about non-normal positions other than (-1,-1) being passed in; these would probably indicate a bug. Both of these can be detected with a well-placed assertion.

The attached patch fixes both of these issues.

Note, the handling here is very similar to that in Ross's corecleanups. I believe this way to be slightly more correct, as it will catch the two pitfall potential bugs mentioned above.

jason
Index: server/plrhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/plrhand.c,v
retrieving revision 1.203
diff -u -r1.203 plrhand.c
--- server/plrhand.c    2001/12/21 11:17:44     1.203
+++ server/plrhand.c    2002/01/02 08:41:17
@@ -893,7 +893,8 @@
   and specified (x,y) coords associated with the event.  Coords will only
   apply if game has started and the conn's player knows that tile (or
   pconn->player==NULL && pconn->observer).  If coords are not required,
-  caller should specify (x,y) = (-1,-1).  For generic event use E_NOEVENT.
+  caller should specify (x,y) = (-1,-1); otherwise make SURE the
+  coordinates have been normalized.  For generic event use E_NOEVENT.
   (But current clients do not use (x,y) data for E_NOEVENT events.)
 **************************************************************************/
 static void vnotify_conn_ex(struct conn_list *dest, int x, int y, int event,
@@ -905,12 +906,16 @@
   genmsg.event = event;
 
   conn_list_iterate(*dest, pconn) {
-    if (is_real_tile(x, y) && server_state >= RUN_GAME_STATE
+    if (server_state >= RUN_GAME_STATE && is_normal_map_pos(x, y)
        && ((pconn->player==NULL && pconn->observer)
            || (pconn->player!=NULL && map_get_known(x, y, pconn->player)))) {
+      /* If is_normal_map_pos(-1,-1), this assertion will catch the bug. */
+      assert(x != -1 || y != -1);
       genmsg.x = x;
       genmsg.y = y;
     } else {
+      /* Any invalid coordinates other than (-1, -1) are a bug. */
+      assert((x == -1 && y == -1) || is_normal_map_pos(x, y));
       genmsg.x = -1;
       genmsg.y = -1;
     }

[Prev in Thread] Current Thread [Next in Thread]