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

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

Lauri Tarkkala <ltarkkal@xxxxxxxxxxxxxxx> wrote:

> I finally rolled up a patch which incorporates all of the work in 
> the previous server i/o patches, except for the "dynamic buffer size
> feature" from vasc. The patch is against the current CVS version.
> 
> The reason for excluding this feature is that the patch is already 
> quite large (~50kB), and is becoming increasingly difficult to produce 
> with an amount of confidence that "nothing broke". 

Yes, for this reason we generally prefer multiple small patches
rather than single large ones.  Although that can get more difficult
when the patches/features are interdependent.

> Therefore I would like to know if there is any interest in including
> this code in CVS, and if not all, then what parts.

Yes, certainly.  Maybe it would have been good to have the zlib
stuff as a separate patch, but may not be worth separating it
now.

Attached is a revised patch which fixes a few things I noticed.
I didn't look in detail at the actual async-io or compression 
stuff.

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

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

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

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

- Remove spurious spacing changes in otherwise untouched files.

- Remove unused 'extern struct connection connections[];' in 
connection.c (which was suspicious in any case because this array
is server-side, so shouldn't be used by common).

- Maybe other minor changes...

Other comments:

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

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

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

-- David

sio-6.diff.gz
Description: GNU Zip compressed data