[Freeciv-Dev] Re: Improved game starting version 6 [patch] [ready, IMO] (PR#1215)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

Daniel L Speyer wrote:

> The sixth version of the game starting patch is now available by
> ftp.  This is mostly cleanup, because I've run out of features that need
> including.  I've been playtesting it for a while, and it works nicely.  I
> think it's ready for inclusion (please review it as such).
Two minor problems:

- When you launch a server "new game", the civclient at first fails to 
connect.  Naturally, this is because the server is just starting up.  I 
don't know if there's a good way around this.
- After playing for a little bit on one game I managed to get the 
control buttons (on the right side of the chat area) to go away.  This 
one may take some time to track down.
- When you choose an empty savegame name, nothing seems to happen.  This 
is a bit confusing.
The interface looks very good; I think its time to seriously consider 
applying this.  But you need to fix the diff problem, of course.  (Why 
does nobody use "cvs diff" around here?)

Now, as to the code: it would be good to have a gui programmer look at 
this, but here's my review.
- A lot of the spacing doesn't follow the style guide.  The #includes 
also aren't grouped correctly in some cases.  Blah, blah, blah...
- game_difficulty and game_aifill in clinet.c shouldn't be made extern 
within connectdlg.c; this is way ugly IMO.  Either put them into 
clinet.h or remove them (somehow) from connectdlg.c.
- The sprintf in clinet.c should probably be an snprintf (or 
mysnprintf).  This may be a matter of style, but it's much easier for 
someone looking at the code to determine it is correct if you use the 
snprintf form.  Otherwise I have to count the characters to make sure 
there's no buffer overflow.
- The code to send /easy, /normal, or /hard in clinet.c could be written 
more elegantly.  Writing it as a switch statement would be better, but 
better still IMO would be something of the form
  *difficulty = (game_difficulty >= 7) ? "/hard" :
                 (game_difficulty >= 5) ? "/normal" :
                  "/easy";
  mystrlcpy(...);
  send_packet_generic_message(...);

- You have a couple of other patches mixed in here.  For instance the 
find_city_near_tile fix is included.
- I notice you still send data over the pipes to civserver.  I still 
think this is bad.  For one thing, there is no feedback on your actions; 
for instance saving a game just silently succeeds/fails.
If the data went entirely over the chatline instead, would it still be 
necessary to have pipes for the civserver's stdin, stdout, and stderr? 
It should just be a matter of making the server happy, right?  How do 
(daemon) servers run on civserver.freeciv.org handle this?
The only thing that looks like it might need to use the pipe is the 
"cmdlevel hack first" line.  This could be controlled from the command 
line using an rc file (though this would be kindof ugly too).
- is_server_running() should IMO be declared with an explicit void 
parameter, e.g.
  int is_server_running(void)
otherwise compilers may think the parameter list just isn't included. 
This also applies to other functions.
-  What's up with this?
+  /*  write(2,"|",1);*/

+  /*  write(2,"%",1);*/

+    /*    write(2,"&",1);*/

- In add_server_ctrl_buttons() the malloc is unnecessary.  If you do use 
malloc it should be fc_malloc.  Again there's the sprintf thing.
ditto in get_player_name.

- In start_server(), the three if() statements at the top can be unified.

- In start_server, shouldn't we write an error message if the exec 
fails?  Or, alternately, how does civclient find out about the failed exec?
- In start_server, your list of execs to find the civserver seems 
adequate, but a better solution may be possible.  But in execvp, the 
first parameter of the argument list is supposed to be the executable - 
which would require changing argv[0] for each execvp.  Ugh.
- In start_server, the if (argv[3]) check is quite a hack.  We may wish 
to add other parameters later, etc.  Why not make this information (if 
we're loading a game) a parameter to the function?
- I see now that if we load a savefile, the cmdlevel hack stuff isn't 
set up.  This could make things more complicated if we try to use the 
chatline for every server command (a player could connect to a game and 
not have enough permissions).  No doubt there's a way around this...
- This is a nit and a style issue to boot, but I don't really like the 
loop used in min_free_port.  What about
for (port=5555; ; port++) {
  ...
  if (/* port is valid */)
    break;
}

- In start_server_for_new_game, rather than use malloc (which will be 
called each time a new game is started from the client, i.e. memory 
leak), why not just declare the buffer static, i.e.
void start_server_for_new_game()
{
  static char port_arg[6];
  /* ... */
  snprintf(port_arg, sizeof(port_arg), "%d", server_port);
  argv[2] = port_arg;
  /* ... */
}

Same in start_server_load_game().  Or perhaps these can be unified into 
one function - a NULL filename meaning no loaded game?  That would make 
sense IMO.
- In filesel_callback2, the free(data) needs a comment of some sort 
(explaining where data is allocated and/or why we're freeing it here). 
Ditto for new_game_ok_callback and new_game_cancel_callback.  In 
filesel_callback there's an unmatched malloc (probably linking to one of 
these, eh?) that could use a comment as well.  Any time the malloc and 
free are separated for local variables some sort of explanation is in 
order IMO.
- In filesel_callback, the 255-length buffer is unsafe.  Not all os's 
will obey this limit.  What about using strdup (or mystrdup or whatever) 
instead?
- Typo: "What do you wish to to?" in popup_main_menu().

- "Quit client only" isn't immediately obvious.  A code comment might be 
helpful; eventually we'll need user documentation.
- What will happen in a GUI that doesn't implement the functions you add 
to connectdlg_g?  It doesn't look like it would be good.  I'll try out 
xaw and see.

Hmmm, that's all.  It looks to me like the functionality is taken care 
of, and all that's needed is refinement/cleanup.
Great patch.

jason