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

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

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
Cc: bugs@xxxxxxxxxxxxxxxxxxx, freeciv-dev@xxxxxxxxxxx
Subject: [Freeciv-Dev] Re: [PATCH] bug in vnotify_conn_ex()
From: Jason Short <vze2zq63@xxxxxxxxxxx>
Date: Thu, 03 Jan 2002 07:51:17 -0500
Reply-to: jdorje@xxxxxxxxxxxx

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



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