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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

You still don't understand that ENOEVENT (-1,-1) is a perfectly valid
case, whether or not your topology allows (-1,-1) as a real tile. It 
is required that this be passed on. There are other cases as well. The
value is used as a signal, independent of topology hardwired into lots 
of code.

Thus you don't fix the original bug, but just kill the server at a
different point. The original problem was ENOEVENT (-1,-1) in a torus
world if you read the comments. They also explain ENOEVENT syntax.

(-1,-1) *is* a special case that *must* just be passed through. All
the asserts that specifically target this are dead wrong. This "fix"
is not only not required but breaks valid behaviour. In fact (-1,-1) 
is the only unnormalized coordinate you absolutely *must* not touch.

Both Raimar and your zeal to stamp out unnormalized coordinates is 
exemplary, but misplaced in this case. Leave this code out of your 
targets! 

Cheers,
RossW
=====

At 02:47 AM 02/01/05 -0500, Jason Short wrote:
>jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>
>> 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. 
>
>This patch is a compromise between Ross's solution and mine.
>
>- I use normalize_map_pos rather than is_normal_map_pos, so if a 
>non-normal map position is passed in we'll be able to deal with it fine 
>(so long as it's not (-1,-1)).  This is basically identical to Ross's 
>changes.
>
>- I've added two extra assertions at the top to catch the two potential 
>bugs I mentioned previously.  These assertions are crucial; without them 
>any buggy code that passes in non-normal positions to this function will 
>result in a nearly untraceable bug on the client end.  NOTE, the use of 
>assert(is_normal(...)) rather than CHECK_MAP_POS(...) is quite 
>intentional - CHECK_MAP_POS may attempt to do error-correction or other 
>fixes rather than a simple assert, which would destroy the point of the 
>code.
>
>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/05 07:39:57
>@@ -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,
>@@ -904,8 +905,27 @@
>   my_vsnprintf(genmsg.message, sizeof(genmsg.message), format, vargs);
>   genmsg.event = event;
> 
>+  /*
>+   * Hack: we special-case the coordinates (-1,-1) to mean the event has
>+   * no coordinates associated with it.  This works just fine, so long
>+   * as (-1,-1) isn't a "real" map position.  But if we use a topology
>+   * where it is a real position, then we have a major problem.  If the
>+   * calling code ever passes in (-1,-1) as the legitimate coordinates of
>+   * the event, we'll end up treating them as the special-case so we'll
>+   * think there are no coordinates associated with the event.  There's
>+   * really no way around this bug, but we can try to catch the error by
>+   * making sure non-normal positions never get passed in.
>+   */
>+  if (x == -1 && y == -1) {
>+    assert(!is_normal_map_pos(x, y));
>+  } else {
>+    assert(is_normal_map_pos(x, y));
>+  }
>+
>   conn_list_iterate(*dest, pconn) {
>-    if (is_real_tile(x, y) && server_state >= RUN_GAME_STATE
>+    if (server_state >= RUN_GAME_STATE
>+        && (x != -1 || y != -1) /* special case, see above */
>+        && normalize_map_pos(&x, &y)
>       && ((pconn->player==NULL && pconn->observer)
>           || (pconn->player!=NULL && map_get_known(x, y, pconn->player)))) {
>       genmsg.x = x;
>