Complete.Org: Mailing Lists: Archives: freeciv-dev: March 2002:
[Freeciv-Dev] Re: Another freeciv testing

[Freeciv-Dev] Re: Another freeciv testing

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: freeciv-dev@xxxxxxxxxxx
Subject: [Freeciv-Dev] Re: Another freeciv testing
From: Jason Short <vze2zq63@xxxxxxxxxxx>
Date: Tue, 12 Mar 2002 06:54:57 -0500
Reply-to: jdorje@xxxxxxxxxxxx

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

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 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;
                "/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.


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