[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 03:44 AM 02/01/02 -0500, Jason Short 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.
[snip]
Remember, is_normal_map_pos() was only deemed useful in asserts and
allowed for the case of CHECK_POS(), i.e. to find passed coordinates
now considered bogus during the process of debugging the new policy.
That was your position, but not mine. I agreed that that was mostly true.
The general rule is ...
is_normal_map_pos() use is almost always a mistake or hiding one :-).
In this case, the mistake is that (-1,-1) is being used to indicate that
there is no tile position, when it is possible (in theory, and hopefully
in practice soon) that (-1,-1) is also a real, valid position. Our
choices then become:
1. Use is_real_tile to check the tile's validity, and special-case
(-1,-1) as a nonexistent position. This is totally wrong IMO; if
non-normal positions are ever passed in eventually (-1,-1) will be
legitimately passed in and will be treated as a nonexistent position. I
am baffled as to why you have chosen this way...
2. Force all map positions passed in to be "normal", and check for
is_normal_map_pos. This is the same as #1 except that we force the
caller to deal with calling normalize_map_pos, and thus avoid the
potential introduced by #1. It _does_ present a problem 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.
3. Do a real cleanup and pass in extra data to let us know whether the
position is relevant. For instance, we could pass in pointer to a
struct map_position (which may be NULL) or simply an extra flag
containing the data (which is better since it can be sent across the
network as such). This would be the "correct" solution, but it would
take more work. Raimar has expressed the opinion that the fix isn't
worth the effort.
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. 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. But if
we force these positions to be "normal" and specify that (-1,-1) can
never be "normal" then there is no ambiguity.
<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! Under an arbitrary topology, when
position (0,0) (or (-1,-1) which would be better) is initialized as the
goto_dest, your code will call normalize_map_pos on it to check for
existence. But this is totally wrong! 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. Again, making
the code "correct" would be the best choice, but choice #1 must be
avoided at all costs. Essentially, you are "initializing" the goto_dest
to just point at some random position on the map.
</change of topic>
Perhaps we can just get off this argument and do the full fix instead?
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 use of assertions is really unnecessary. This is a reasonable point
to do full checks and fixups as an entry/exit data portal. Killing a server
everytime a client does something bogus is not good form.
This data is being sent to the client, not taken from it. It is the
server that is generating these positions, and it should not be possible
for the client to get the server to generate a bogus position. But if
it is possible, then this is the place to catch it.
The original code did full checks. There might actually be a reason for
this, other than happenstance. Certainly it has not caused problems, so
there is no reason to change it here.
Currently it is safe to pass in (-1,-1) to mark a "non-existent"
position, because (-1,-1) will always be unreal. Under different
topologies, (-1,-1) will not always be unreal. Thus, a change is necessary.
The reason the original code did the full check is that coordinates were
not assumed to be normal when they were passed in, so the code knew it
had better check for full realness (and normalize them too). This is
the situation that CHECK_MAP_POS has changed. You may argue that the
change is unnecessary, but it has three definite advantages: (1) less
code overall, (2) the code is faster, and (3) hacks like this will work
without introducing bugs.
jason
[Freeciv-Dev] Re: [PATCH] bug in vnotify_conn_ex(), Raimar Falke, 2002/01/05
|
|