[Freeciv-Dev] Re: PATCH: network i/o

[Lauri Tarkkala <ltarkkal@xxxxxxxxxxxxxxx>]

On Mon, Sep 04, 2000 at 03:44:20PM +1100, David Pfitzner wrote:
> Changes:
> 
> - Changed the capability string and zlib negotiation stuff.  Although
> on the one hand zlib support does seem like a "capability", I think
> its nicer in some respects not to include compile-time options in
> the capability string.  Instead I put a "negotiate_zlib" tag in
> the capability string.  The client sends an extra byte in the
> join_game_request (will be ignored by older servers), which is
> 1 if client has zlib support.  Server bases reply on this and
> game.net_compress_level and "negotiate_zlib" in capability.
> (Added has_zlib_support to connection struct, only used by server.)

This is probably a nicer way to implement it, all things considered.

My approach was probably slightly too much "quick fix".
 
> - Also, in join game reply, changed net_compress in packet from 
> uint32 to uint8, and moved it to genuine end-of-packet (after 
> capability-dependent conn_id).

"Oops" 

> - You didn't have any code to enfore 'netcompress==2' option; added.
> Ie, reject non-zlib clients if specify that zlib required.

Uhhuh.. My mistake. I did write code for this in handle_request_join_game()
but apparently I fumbled the CVS merge and the code did not get included
in the patch :( 

> - Readline completion didn't work for 'list clistat', mainly because
> the current handling of 'list' arguments is pretty hacky.  I made it
> similar to other cases (enum, array of strings, etc).
> 
> - Moved show_clistat() to after show_players() and show_connections(),
> and some minor changes.

> - The max values for tcptimeout and netwait seem small to me?

These values have been generated by simply guessing according to
the following rationale.

The values are in seconds, so both maximums are 2 minutes. IMHO
after this it doesn't really matter if the timeouts are "forever" or
less. After ~4-5 minutes TCP should cut the connection anyway, but they
probably ought to be increased. Changing them to say 600 probably is
enough.

> - I found it a bit confusing that game.net_compress_level is named so
> similarly to game.save_compress_level when actually the 'levels' are
> quite different: latter is "compression level/quality" 1 to 9 (or 0
> for off), while former is an enum of whether compression is allowed
> and/or required for clients.  Didn't change though.

Yeah.. my mistake.

> - The maphand.c changes had me very confused, but I eventually
> realised the strange changes (ie, "separating" the y and x looping)
> were maybe intentional (avoid over-filling buffers??)  Should be 
> commented though...

The code is from the "freeciv-net2.patch.gz" by Vasc. I am not
familiar with this code.

As I recall our conversations on #freeciv vasc was working on avoiding 
overflowing the buffers. As the writes were no longer synchronous, 
less time for the write()'s to finish before next write() -> buffers 
overflowed. I therefore assume your interpretation is correct.

The patch seems pretty much fine to me. I did some quick tests on the 
sio-6.diff,
and the zlib code seems to work ok. Basic i/o seemed to work ok (played a 
game with AI's). Testing the "netwait" and "tcptimeout" parameters 
is probably going to take more work.

Lauri

-- 
"The credit belongs to the man in the arena whose face is marred by dust and
sweat and blood, who strives valiantly, who errs, and who comes up short again
and again, who knows the great enthusiasms, the great devotions, and spends
himself in a worthy cause. The man who at best knows the triumph of high
achievement and who at worst, if he fails, fails while daring greatly, so that
his place will never be with those cold timid souls who never knew victory or
defeat." - Theodore Roosevelt