[Freeciv-Dev] Re: Another freeciv testing

[Daniel L Speyer <dspeyer@xxxxxxxxxxx>]

On Tue, 12 Mar 2002, Jason Short wrote:

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

There?? weird.  I thought I'd caught all those wrong numbers.  I'll switch
it over to send_server_commandline, though, that'd be cleaner in any case.

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

Disconnect definately shouldn't kill the server, but it should get rid of
those buttons.  I wonder if I should make the whole thing use
gtk_widget_hide instead of building and destroying them.  That might
squash some other weird bugs.

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

No, they have to be where they are.  Or, at least, the declaration has to
be in common code and the extern has to apply to gui-gtk/connectdlg.c.  I
could move the externs into the .c file, I suppose, but that would be
it.  The whole point of those variables is that the common code needs to
get data from the gui-specific code *whether or not the gui-specific code
knows to send the data*.  Maybe I should add some comments explaining
this.

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

Yeah, I guess I should tighten that.

> 
> 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 *should* raise such an error.  I don't know how to make it raise
one.  In any case, however, it's better to get the compiler error
"included file wait.h not found" than the compiler error "warning,
implicite declartion of int wait(int)" followed by a half-readable linker
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).
> 

Since pipe() wants an array anyway, it seems more elegant just to give it
one, than to do extra data copying.  At one point I was sending data
through those extra handles (not the prettiest, but sometimes a good
idea), and, while that code is removed, it might be desired again.

What's to be gained by close()ing them?

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

Yeah, I should drop that.

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

Hmm, it's probably correct then.  I'll change it.

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

I started the patch before freeciv style existed.  It'll take a while to
switch it over.

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

Well, I'd rather get the code to as many users as possible as quickly as
possible :), but there may be good reason in going to gtk 2.  I haven't
looked at it yet, but it can't be too different.  Does the gui-gtk2
maintainer have an opinion?

--Daniel Speyer
If you *don't* consider sharing information to be morally equivalent to 
kidnapping and murder on the high seas, you probably shouldn't use the
phrase "software piracy."

> 
> jason
> 
> 
>