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

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

At 08:00 PM 02/01/05 -0500, Jason Short wrote:
>Ross W. Wetmore wrote:
>
>> 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! 
>
>I think you are misreading the code.  (-1,-1) _is_ treated as a 
>special-case for a nonexistent tile.  It fails the check and so is 
>passed through as (-1,-1) directly to the client.

Sorry, but in a torus world your assert kills the server. Remove the
assert for the (-1,-1) case and just pass it through untouched, unchecked
and however unloved. It does not need to be 1984ed or purified by the
normalizing assert benediction.

You have still changed the behaviour with the assert for non-special
cases which were once allowed and now are not. There is no reason to
do this here other than misplaced zeal. There is *never* any reason
to force normalization other than at the original generation point
or before you access an interface that requires normalized values.
None exist here, ergo it is not needed.

Note, non-normal coordinates are not a problem because they can 
*always* be fixed at any point in the code in a completely standard
fashion - this is the way everything used to work and it still works
if you need it.

Unreal coordinates *are* a problem because only the generator of the
unreal position has any reasonable chance of dealing with the failure
codepath. Hence is_normal_map_pos() is usually a mistake because it
misses fixable problems, and cannot detect the unfixable ones.

>The problem is that if (-1,-1) is passed in as a legitimate set of 
>coordinates, it will be treated as the special-case of no coordinates 
>associated with the event.  Thus it is very important that we stamp out 
>non-normal coordinates being passed to this function!  Of course, we 
>must allow (-1,-1) to be passed but if any _other_ non-normal 
>coordinates are passed in then we're in danger.  This is a bug in the 
>calling code that can only be caught here.
>
>jason

>+  /*
>+   * 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. 
                          ^^^^^^
In a torus world, -1,-1 is a real position. ENOEVENT(-1,-1) is a perfectly
valid event.

There is no reason not to include (-1,-1) in a normal set because as you 
keeping saying, it is arbitrary how you define "normal", unlike real
which does have a hard meaning.

> 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. 

This is not the problem here. This is not the place to fix it. Code that
cannot distinguish a real coordinate from a signal may need to be updated.

But I suggest you go find some before you "fix" something that isn't
shown to be broken. There is context you don't even seem to recognize
here that can be used to disambiguate signals from real.

> There's
>+   * really no way around this bug, but we can try to catch the error by

There is no bug here.

>+   * making sure non-normal positions never get passed in.

This creates a new class of bugs at this point.

>+   */
>+  if (x == -1 && y == -1) {
>+    assert(!is_normal_map_pos(x, y));

Torus world, (-1,-1) is a perfectly real and quite possibly normal
coordinate. Kaboom. Do you read this code differently?

>+  } else {
>+    assert(is_normal_map_pos(x, y));
>+  }

Your fix is not required, dangerous in the hands of incompetents or 
even competent future developers that don't share your misguided zeal
and do the conceptually and architecturally correct thing. Therefore 
there is absolutely no hard reason for it to be made, no downside
for not doing it. So, just don't do it.

The rest of the fix is fine now.

Cheers,
RossW
=====