[Freeciv-Dev] Re: [PATCH] bug in vnotify_conn_ex()
[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
Ross W. Wetmore wrote:
At 07:51 AM 02/01/03 -0500, Jason Short wrote:
Ross W. Wetmore wrote:
At 03:44 AM 02/01/02 -0500, Jason Short wrote:
[a lot of excerpts...]
if
non-normal positions are ever passed in eventually (-1,-1) will be
legitimately passed in and will be treated as a nonexistent position.
Non-normal is meaningless. I repeat, non-normal is meaningless, I repeat
non-normal is meaningless ... get it through your skull :-)
"Normal" only has meaning because we choose to use it; it has no
inherent value. But to rephrase...
If the position (-1,-1) is allowed to be passed in as a legitimate
position, it will be impossible for the code (either server code in
vnotify_conn_ex or the client code that receives the packet) to
determine whether this (-1,-1) is a legitimate real position or a
special-case to mark the even as having no coordinates associated with
it. This is a bug, and a very hard one to track down.
OTOH, if we say that the legitimate positions passed in must be "normal"
and specify that (-1,-1) can never be "normal", then we don't have this
problem. However, code may pass in legitimate "non-normal" positions,
causing the code to fail...but since we'll get a failed assertion in
this case the bug is very easy to track down.
Of course, I still maintain that using a more correct solution rather
than the (-1,-1) hack would be better.
However, I think a compromise that will please both of us may be
possible. I'll send the patch tomorrow.
because on the
client side the set of normal positions may be different, but it's safe
enough to special-case (-1,-1) there because we trust that the server
has taken care of any problems.
Only for the clients you can fix. You break all current clients by this
which is not particularly good policy. Current clients send various
event/packets and expect various event/replies. Part of what they
understand is how to ignore or deal with (-1,-1).
Current clients will not be broken. (-1,-1) will continue to be used as
the special-case for non-existent coordinates, while any other
coordinates will be guaranteed to be normal. As I've said, such a
gurantee is not necessary for the current topology, but it _is_
necessary for any topology in which (-1,-1) can be real.
Of course, current clients _will_ be broken by a change of topologies,
but that's unavoidable AFAICT.
Now, you may point out that all we're doing is sending (-1,-1) as the
position to the client in place of (-1,-1). But suppose such an event
happens and (-1,-1) is sent to the client.
This is what the code is currently designed to do ... send (-1,-1) to
the client. Your concern about "sending (-1,-1) ... in place of (-1,-1)"
definitely seems a few marbles short of a full set :-).
To repeat (again), the current code sends (-1,-1) which has a
well-defined meaning. If (-1,-1) is a "real" position then it has two
possible meanings.
Should the client think that
this event has no coordinates associated with it (for instance, if it
happens out-of-sight), or that the coordinates (-1,-1) are associated
with the event? If we use your system, there is no way to tell.
Not my system ... it is THE CURRENT SYSTEM. That's the way it is designed
to work, the way it is currently coded to work.
But only under the current topology.
But if
we force these positions to be "normal" and specify that (-1,-1) can
never be "normal" then there is no ambiguity.
It works fine as is/was. The correct restoration fix has been thoroughly
exercised in both Torus world and current scenarios and there is no problem
here that needs any "forcing".
Only because the calling code only passes in normal positions anyway.
As long as (-1,-1) is never passed in, there is no problem.
But even if non-normal positions were passed in, you still probably
wouldn't notice a problem. Things would only fail if (-1,-1) were
passed in, and even then I'm not quite sure what the result would be.
The client would probably just treat the event slightly wrongly, and
only very rarely would anyone notice. The bug would never be fixed.
<change of topic>
When we talk about the similar goto_dest problem, the use of #1 is even
worse, because the solution you propose doesn't even special-case out
the "nonexistent position" values!
It most certainly does! Go figure out what normalize_map_pos() does.
Even in the current buggy version it still does this :-).
Normalize_map_pos() is what is_normal_map_pos() keeps trying to be, but
can't because it doesn't understand the critical hard condition realness.
No. Normalize_map_pos may return 1 on (-1,-1), whereas
is_normal_map_pos will return 0. This is really the only quality we're
interested in here.
A position is either (1)
non-existent, in which case the coordinates will be a fixed (0,0) which
may either be real or not or (2) existent, in which case the coordinates
had better be real and should probably be normal as well.
A position is either unreal, in which case you can't do anything with
it and better have coded the failure path, or it is real. This is an
unambiguous hard condition requiring real separate codepaths.
But the "hack" introduces a third possiblity, in which the position is
(-1,-1) which may be real, but does not refer to a real tile. The only
way to avoid this is to fix the hack.
In these, cases you normalize it. Since a correctly functionning
normalize_map_pos() does all this, tests for realness, tests for normal
and normalizes only if necessary it can be called to return the unreal
condition, or the correct useful coordinates with which to continue.
The end result is that the goto_dest will be initialized to (-1,-1)
(indicating that there is no goto_dest), normalize_map_pos will be
called on it, and we will end up with a goto_dest pointing to (for
instance) (32, 45). If there happens to be a friendly city on that
tile, our unit (well, actually _everyone's_ units) will try to attack
that position. Again, the bug will almost never manifest itself because
the goto_dest will quickly be changed, and it will probably not be fixed
for a long, long time.
</change of topic>
Perhaps we can just get off this argument and do the full fix instead?
Full fix to what?
By "full fix" I mean not special-casing (-1,-1) to mean a nonexistent
position, but instead having a separate flag to indicate this. This
will require changing a fair amount of code to get the server to work,
then changing the communications protocol (and adding a new capability),
then changing the client to accept the new data.
jason
[Freeciv-Dev] Re: [PATCH] bug in vnotify_conn_ex(), Raimar Falke, 2002/01/05
|
|
