[Freeciv-Dev] Re: Freeciv Networking/Mac Server patches

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

On Sun, 20 Aug 2000 Andy Black <ablack@xxxxxxxxxxxxxx> wrote:
> >
> >> mem.c.diff
> >>    a few mac (client) related items.
> >> mem.h.diff
> >>    proto for failure_cleanup
> >
> >Since failure_cleanup() doesn't currently do anything and is not
> >called from anywhere else, I'm not that keen on adding it...
> 
> failure_cleanup() is used in the mac client.  I have a file pair called
> mac_mem.[c/h] that is similar to mem.c/h, except for mac toolbox memory
> allocation calls.  I don't know if it is actually needed.  The reason I
> created it was the folowing comment in handle_alloc_failure
>   /*
>    * Do anything else here (cleanups? safe-save??)
>    */
> mac_mem.c includes a routine called handle_mac_mem_failure.  It is similar
> to handle_alloc_failure, but includes the return of the mac toolbox call in
> the log.  My reasoning had been that any cleanup would probably happen
> roughly the same way, regardless of platform.

Well, I still don't much like adding an empty function, just so that it
mirrors the structure of similar code for one of the clients.  If we
find something to add, we can add it then (or simply add code directly
in handle_alloc_failure(), no need for another function?).

Regarding calling platform-specific memory functions, an appropriate
way to do this would be to change mem.[ch] so caller can register
callback functions to use, for allocation and/or failure.  (Well, the
caller-supplied allocation routine could handle failure however it
likes, although probably also want to return NULL and have
general-case processing after that.)  (Compare log_init() and
close_socket_set_callback() for other cases of registering callbacks.)

(The advantage of this approach is that you can then be assured that
all code uses consistent memory allocation, whether it is called from
client, or from common via client, etc.)

> aitech.c.diff
>       alternate memory allocation. needed to compile.  (please comit)

Sorry, but this patch was pretty gross, IMO.  Freeciv is mostly
portable C, except where necessary (networking, GUI, necessary
OS-stuff etc), and should _not_ have #if blocks added except where
necessary.

Secondly, for such a small patch it had quite a few errors!  :-7
- sizeof(char) is always exactly 1 (see below), so you don't need to 
  use it explicitly (ok, not an error, but style IMO);
- but actually, many of those sizeof(char) needed to be sizeof(int)!
- 'cache' variable is/was multi-dimensional array, so need to either do 
  multi-level allocation (allocate pointers and then allocate for each), 
  or change code which uses cache to use 'flat' allocation (see patch);
- when change from array to allocated pointer, need to change code like:
      memset(values, 0, sizeof(values));
  because sizeof is now the size of the pointer, not the array, so this
  will break.

I'm assuming only "unsigned char cache[A_LAST][A_LAST]" was the real
problem, (size 40,000 bytes, > 32,768 bytes?), so attached patch changes
only this one (for all platforms), to allocated (once, static, never
freed).  (Also renamed unrelated variable 'c' to descriptive name.)

> aiunit.c.diff
>       mac keyword conflict change. (please comit)

Done.

> civserver.c.diff
>       updated mac option dialog.  bypased with hack due to crash problems.

Done.

> support.c.diff
>       adition of my_gethostname(char string[]).  size paramater of
> gethostname is suposedly deduced inside.  NOT TESTED.  I'll fix it (add
> length paramater) if anyone has problems.

Guess what - I have problems with it!  

You cannot use sizeof like that on functions arguments, even if you
declare the argument as an array and pass it an array. (Eg, see
http://www.eskimo.com/~scs/C-faq/q6.4.html, and q6.21.html).  Also, 
I think its best for my_gethostname() to look as much like the real
gethostname() as possible, in general.  Modified patch attached
(second patch).

As mentioned above, I disagree that sizeof(char) can be anything
other than 1.  Even if char is not 8 bits, that just means a "byte"
(as far as C goes) is not 8 bits on such a platform (<trivia> thats
which is why some docs say "octet" for 8 bits, instead of "byte").
Ref: K&R 2nd ed p204 (A7.4.8); also 
http://www.eskimo.com/~scs/C-faq/q7.8.html

> diplhand.c.diff
>       security patches and lots of rambling comentary. (please comit)

(Todo; have to look at how this interacts with diplhand patches by
others.)

Regarding the networking stuff, from a brief look I am concerned about
the common interface and whether the "BSD networking" part can be made
to work within the proposed framework.  Eg, where you have:
   /*select() here?*/
yes, you need _something_ there, but I'm not sure that select or
anything else is going to work appropriately in that particular
situation.  (Eg, _maybe_ non-blocking sockets and a busy-loop outside,
but thats not very nice.  Is that what its supposed to do in this
patch?)  Also the stdin stuff is intimately tied in with select(), so
correctly moving it to console.c is non-trivial at best...

-- David

mac_aitech.diff.gz
Description: GNU Zip compressed data

my_gethostname.diff.gz
Description: GNU Zip compressed data