[Freeciv-Dev] Re: Another freeciv testing

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Daniel L Speyer wrote:
> On Mon, 11 Mar 2002, Michael Suess wrote:
>
>
> > > OK, try this version, which should apply cleanly against the current
> > > CVS.  It has a few minor bugfixes in it as well.
> > It applies cleanly, and compiles as well, however my loading problems remain as
> > told...
> Hmmm, try this then.  It sticks timeouts onto all busy-wait loops, which
> I'm not really comfortable with, but should eliminate all freezish
> problems (that's what you had, right?  I'm starting to mix bugs together
> in my memory).
Some comments on the new patch:


I, too, get a hang when I try to load a game.  After I choose the save 
file, the dialog correctly pops up allowing me to choose my player.  But 
when I choose a player it immediately hangs.  The problem is in this line:
+  write(stdin_pipe[1],"cmdlevel hack first\n",14);

where the 14 is clearly not correct.  To avoid ugly-as-hell problems 
like this, use send_server_commandline() (which is currently unused) for 
all such writes.  When I change it like so, it works correctly.

However, there is another problem, which is that when you (D)isconnect 
(from the Game menu) we end up in a bad state.  The server is not 
killed, which is (I guess) a good thing.  But also the server control 
buttons are not removed...so that, for instance, you can load a game, 
disconnect, load it again, and end up with two copies of the server 
control buttons.

game_difficulty/game_aifill/game_autotoggle/game_autostart are declared 
in clinet.c, but declered extern in connectdlg.h.  They should go into 
the same files; i.e. into either client.[ch] or (more sensible IMO) 
connectdlg.[ch].

The following code

  if (game_aifill!=-1){
    char buf[15];
    struct packet_generic_message apacket;
    snprintf(buf,15,"/set aifill %d",game_aifill);
    mystrlcpy(apacket.message, buf, MAX_LEN_MSG-MAX_LEN_USERNAME+1);
    send_packet_generic_message(&aconnection, PACKET_CHAT_MSG,&apacket);
  }

could just be

  if (game_aifill!=-1){
    struct packet_generic_message apacket;
    my_snprintf(apacket.message,
                MAX_LEN_MSG-MAX_LEN_USERNAME+1,
                "/set aifill %d",game_aifill);
    send_packet_generic_message(&aconnection, PACKET_CHAT_MSG,&apacket);
  }

Not that this matters at all...


What should be done about HAVE_SYS_WAIT_H?  Are you saying its absense 
*does* raise an error at configure time (only if enable-client=gtk), or 
that it *should* raise such an error?

It isn't necessary (and is confusing IMO) to track both the server and 
client's pipe FD's from the client.  Declare the std**_pipe[] arrays 
directly within start_server(), and only track our (the client's) FD end 
of the connection globally.  After the fork, close the other end of the 
pipe in both parent and child code (don't wait for the server to exit 
before doing this at the parent end).

Why pipe stderr at all?  You pipe it, but don't do anything with the 
output.  This could be bad if a lot of data was sent to stderr (not 
sure).  More to the point, if you leave stderr as-is it will send output 
to stderr, where the user will see it...which seems like a good idea to 
me.  An alternative would be to call close(2) after the fork, but this 
would probably give Bad Results when the server opened up a connection 
socket on stderr.

The rest of freeciv uses response instead of responce (although this is 
just a few uses).  It'd be good to be consistent...

Make sure you use fc_malloc() instead of malloc(), and my_snprintf 
instead of sprintf/snprintf.  And make sure you use freeciv coding 
style, mostly "x = 5;" instead of "x=5;"

As for all the GTK stuff...well, you're the GTK coder.  I don't have the 
energy to wade though it again :-).

Finally, a thought: you might have more luck getting this into 
gui-gtk-2.0 instead of gui-gtk.  I imagine the gtk-2.0 code will undergo 
some rapid changes in the near future, and having the cool connection 
dialog would give people incentive to upgrade.

jason