[Freeciv-Dev] (PR#8491) Re: Freeciv commit: kauf: A new connect dialog for the client. This allo...

["Raimar Falke" <i-freeciv-lists@xxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=8491 >

On Fri, Apr 09, 2004 at 09:00:42PM -0700, Raimar Falke wrote:

There are several problems in this commit. Some minor some more
serious. They should all be fixed.

> +/************************************************************************** 
> +The general chain of events:
> +
> +Two distinct paths are taken depending on the choice of mode: 
> +
> +if the user selects the multi- player mode, then a packet_req_join_game 
> +packet is sent to the server. It is either successful or not. The end.
> +
> +If the user selects a single- player mode (either a new game or a save game) 
> +then: 
> +   1. the packet_req_join_game is sent.
> +   2. on receipt, if we can join, then a challenge packet is sent to the
> +      server, so we can get hack level control.
> +   3. if we can't get hack, then we get dumped to multi- player mode. If
> +      we can, then:
> +      a. for a new game, we send a series of packet_generic_message packets
> +         with commands to start the game.
> +      b. for a saved game, we send the load command with a 
> +         packet_generic_message, then we send a PACKET_PLAYER_LIST_REQUEST.
> +         the response to this request will tell us if the game was loaded or
> +         not. if not, then we send another load command. if so, then we send
> +         a series of packet_generic_message packets with commands to start 
> +         the game.
> +**************************************************************************/ 

There is no packet_generic_message.

> +/**************************************************************** 
> +forks a server if it can. returns FALSE is we find we couldn't start
> +the server.
> +This is so system-intensive that it's *nix only.  VMS and Windows 
> +code will come later 
> +*****************************************************************/ 

AFAIK the comments are written in English. AFAIK every English
sentence starts with a capitalized word. This is the problem of the
whole commit.

> +  argv[i] = fc_malloc(10);
> +  my_snprintf(argv[i++], 10, "civserver");
> +  argv[i] = fc_malloc(3);
> +  my_snprintf(argv[i++], 3, "-p");
> +  argv[i] = fc_malloc(7);
> +  my_snprintf(argv[i++], 7, "%d", server_port);

I would have written this with a buffer:

my_snprintf(buffer, sizeof(buffer), "civserver");
argv[i] = mystrdup(buffer);
...
Less numbers which you can get wrong.

> +  server_pid = fork();
> +  
> +  if (server_pid == 0) {
> +    int fd;
> +
> +    /* inside the fork */  

"the child".

> +    /* avoid terminal spam, but still make server output available */ 

add "by redirecting the output to the logfile".

> +    /* these won't return on success */ 
> +    execvp("./ser", argv);
> +    execvp("./server/civserver", argv);
> +    execvp("civserver", argv);

> +    /* This line is only reached if civserver cannot be started, 
> +     * so we kill the forked process.

> +     * Calling exit here is dangerous due to X11 problems (async replies) */

We use exit in a lot of other places. Can someone provide further
informations?

> +    _exit(1);

> +  /* don't need these anymore */ 

Add "don't free the terminating NULL".

> +  for (i = 0; i < nargs - 1; i++) {
> +    free(argv[i]);
> +  }
> +  free(argv);
> +
> +  /* a reasonable number of tries */ 
> +  while(connect_to_server((char *)user_username(), "localhost", server_port, 
> +                          buf, sizeof(buf)) == -1) {
> +    myusleep(WAIT_BETWEEN_TRIES);
> +

> +    if (i > NUMBER_OF_TRIES)  break;

Add {}

> +    i++;
> +  }

> +  /* weird, but could happen, if server doesn't support new startup stuff
> +   * capabilities won't help us here... */ 
> +  if (!aconnection.used) {

> +    /* possible that server is still running. kill it */ 
> +    kill(server_pid, SIGTERM);
> +    server_pid = -1;

Use function client_kill_server.

> +    append_output_window(_("Couldn't connect to the server. We probably "
> +                           "couldn't start it from here. You'll have to "
> +                           "start one manually. Sorry..."));
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +#else /*VMS or Win32*/
> +  return FALSE;
> +#endif
> +}
> +
> +/************************************************************************** 
> +Finds the lowest port which can be used for the 
> +server (starting at the 5555C)
> +**************************************************************************/ 
> +int min_free_port(void)
> +{
> +  int port, n, s;
> +  struct sockaddr_in tmp;
> +  
> +  s = socket(AF_INET, SOCK_STREAM, 0);
> +  n = INADDR_ANY;
> +  port = 5554; /* make looping convenient */ 
> +  do {
> +    port++;
> +    memset(&tmp, 0, sizeof(struct sockaddr_in));
> +    tmp.sin_family = AF_INET;
> +    tmp.sin_port = htons(port);
> +    memcpy(&tmp. sin_addr, &n, sizeof(long));
> +  } while(bind(s, (struct sockaddr*) &tmp, sizeof(struct sockaddr_in)));
> +
> +  my_closesocket(s);
> +  
> +  return port;
> +}
> +
> +/**************************************************************** 
> +if the client is capable of 'wanting hack', then the server will 
> +send the client a filename in the packet_join_game_reply packet.
> +
> +this function creates the file with a suitably random number in it 
> +and then sends the filename and number to the server. If the server 
> +can open and read the number, then the client is given hack access.
> +*****************************************************************/ 

> +void send_client_wants_hack(char * filename)

Add "const".

> +{
> +  struct section_file file;
> +
> +  /* find some entropy */ 
> +  int entropy = myrand(MAX_UINT32) - time(NULL);

> +  /* we don't want zero */ 
> +  if (entropy == 0) {
> +    entropy++;
> +  }

Why?

> +  section_file_init(&file);
> +  secfile_insert_int(&file, entropy, "challenge.entropy");
> +  section_file_save(&file, filename, 0);      
> +  section_file_free(&file);
> +
> +  /* tell the server what we put into the file */ 
> +  dsend_packet_single_want_hack_req(&aconnection, entropy);
> +}
> +
> +/**************************************************************** 
> +handle response (by the server) if the client has got hack or not.
> +*****************************************************************/ 
> +void handle_single_want_hack_reply(bool you_have_hack)
> +{
> +  if (you_have_hack) {

> +    append_output_window("We have control of the server "
> +                         "(command access level hack)");

i18n

> +    client_has_hack = TRUE;
> +  } else if (is_server_running()) {
> +    /* only output this if we started the server and we NEED hack */

> +    append_output_window("We can't take control of server, "
> +                         "attempting to kill it.");

i18n

> +    client_kill_server();
> +  }
> +}
> +
> +/**************************************************************** 
> +send server commands to start a saved game.
> +*****************************************************************/ 
> +void send_start_saved_game(void)
> +{   
> +  char buf[MAX_LEN_MSG];
> +
> +  send_chat("/set timeout 0");
> +  send_chat("/set autotoggle 1");
> +  my_snprintf(buf, sizeof(buf), "/take %s %s", user_name, player_name);
> +  send_chat(buf);
> +  send_chat("/start");
> +}
> +
> +/**************************************************************** 
> +send server command to save game.
> +*****************************************************************/ 

> +void send_save_game(char *filename)

Add "const"

> +{   
> +  char message[MAX_LEN_MSG];
> +
> +  if (filename) {
> +    my_snprintf(message, MAX_LEN_MSG, "/save %s", filename);
> +  } else {
> +    my_snprintf(message, MAX_LEN_MSG, "/save");
> +  }
> +
> +  send_chat(message);
> +}
> Index: client/connectdlg_common.h
> ===================================================================
> RCS file: client/connectdlg_common.h
> diff -N client/connectdlg_common.h
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ client/connectdlg_common.h        10 Apr 2004 03:47:48 -0000      1.1
> @@ -0,0 +1,44 @@
> +/********************************************************************** 
> +Freeciv - Copyright (C) 2003 - The Freeciv Project
> +   This program is free software; you can redistribute it and / or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2, or (at your option)
> +   any later version.
> +
> +   This program is distributed in the hope that it will be useful, 
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +***********************************************************************/ 
> +#ifndef FC__CONNECTDLG_COMMON_H
> +#define FC__CONNECTDLG_COMMON_H
> +
> +#include "shared.h"
> +
> +bool client_start_server(void);
> +void client_kill_server(void);
> +bool is_server_running(void);
> +
> +int min_free_port(void);
> +
> +void send_client_wants_hack(char *filename);
> +void send_start_saved_game(void);
> +void send_save_game(char *filename);
> +
> +extern char player_name[MAX_LEN_NAME];
> +extern char *current_filename;

> +enum skill_levels { 
> +  NOVICE, 
> +  EASY, 
> +  NORMAL, 
> +  HARD, 
> +  EXPERIMENTAL,
> +  NUM_SKILL_LEVELS
> +};
> +
> +extern const char *skill_level_names[NUM_SKILL_LEVELS];

Shouldn't/can't these be shared with the server.

> +extern bool client_has_hack;
> +
> +#endif  /* FC__CONNECTDLG_COMMON_H */ 


> Index: client/include/connectdlg_g.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/include/connectdlg_g.h,v
> retrieving revision 1.5
> retrieving revision 1.6
> diff -u -r1.5 -r1.6
> --- client/include/connectdlg_g.h     29 Nov 2003 09:52:45 -0000      1.5
> +++ client/include/connectdlg_g.h     10 Apr 2004 03:47:49 -0000      1.6
> @@ -14,6 +14,7 @@
>  #define FC__CONNECTDLG_G_H
>  
>  void close_connection_dialog(void);

> +void really_close_connection_dialog(void);

The naming is very poor. The documentation don't make it any clearer
what this function should do.

> +/******************************************************************
> + reinitialize the options_settable struct: allocate enough
> + space for all the options that the server is going to send us.
> +*******************************************************************/
> +void handle_options_settable_control(
> +                               struct packet_options_settable_control 
> *packet)
> +{
> +  int i; 
> +
> +  settable_options_free();
> +

> +  options_categories = fc_malloc(packet->ncategories * sizeof(char *));

Here and in a lot of other places: don't use sizeof of an explicit
type. Use the variable you assign to:

options_categories =
  fc_malloc(packet->ncategories * sizeof(options_categories[0]));
or
options_categories =
  fc_malloc(packet->ncategories * sizeof(*options_categories));

> +  num_options_categories = packet->ncategories;
> +  
> +  for (i = 0; i < num_options_categories; i++) {
> +    options_categories[i] = mystrdup(packet->category_names[i]);
> +  }

> +  /* avoid a malloc of size 0 warning */
> +  if (packet->nids == 0) {
> +    return;
> +  }

You don't set num_settable_options to 0 here.

> +  settable_options = fc_malloc(packet->nids * sizeof(struct 
> options_settable));
> +  num_settable_options = packet->nids;
> +
> +  for (i = 0; i < num_settable_options; i++) {
> +    settable_options[i].name = NULL;
> +    settable_options[i].short_help = NULL;
> +    settable_options[i].extra_help = NULL;
> +    settable_options[i].strval = NULL;
> +    settable_options[i].default_strval = NULL;
> +  }
> +}


> +/******************************************************************
> + Fill the settable_options array with an option.
> + If we've filled the last option, popup the dialog.
> +*******************************************************************/
> +void handle_options_settable(struct packet_options_settable *packet)
> +{
> +  int i = packet->id;

> +  assert(i >= 0);

Also check against the upper bound.

> +  settable_options[i].name = mystrdup(packet->name);
> +  settable_options[i].short_help = mystrdup(packet->short_help);
> +  settable_options[i].extra_help = mystrdup(packet->extra_help);
> +
> +  settable_options[i].type = packet->type;
> +  settable_options[i].category = packet->category;

> +  switch (packet->type) {
> +  case 0:

Use enums.

> +    settable_options[i].val = packet->val;
> +    settable_options[i].min = FALSE;
> +    settable_options[i].max = TRUE;
> +    settable_options[i].strval = NULL;
> +    settable_options[i].default_strval = NULL;
> +    break;
> +  case 1:
> +    settable_options[i].val = packet->val;
> +    settable_options[i].min = packet->min;
> +    settable_options[i].max = packet->max;
> +    settable_options[i].strval = NULL;
> +    settable_options[i].default_strval = NULL;
> +    break;
> +  case 2:
> +    settable_options[i].strval = mystrdup(packet->strval);
> +    settable_options[i].default_strval = mystrdup(packet->default_strval);
> +    break;
> +  default:
> +    assert(0);
> +  }
> +
> +  /* if we've received all the options, pop up the settings dialog */
> +  if (i == num_settable_options - 1) {
> +    popup_settable_options_dialog();
> +  }
>  }
> Index: common/connection.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/connection.h,v
> retrieving revision 1.33
> retrieving revision 1.34
> diff -u -r1.33 -r1.34
> --- common/connection.h       14 Jan 2004 11:58:12 -0000      1.33
> +++ common/connection.h       10 Apr 2004 03:47:49 -0000      1.34
> @@ -36,6 +36,7 @@
>  struct timer_list;
>  
>  #define MAX_LEN_PACKET   4096
> +
>  #define MAX_LEN_BUFFER   (MAX_LEN_PACKET * 128)
>  #define MAX_LEN_CAPSTR    512

Noise.

> Index: common/dataio.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/dataio.h,v
> retrieving revision 1.4
> retrieving revision 1.5
> diff -u -r1.4 -r1.5
> --- common/dataio.h   28 Nov 2003 17:37:21 -0000      1.4
> +++ common/dataio.h   10 Apr 2004 03:47:49 -0000      1.5
> @@ -59,7 +59,8 @@
>  
>  void dio_get_sint8(struct data_in *din, int *dest);
>  void dio_get_sint16(struct data_in *din, int *dest);
> -void dio_get_sint32(struct data_in *din, int *dest);
> +#define dio_get_sint32(d,v) dio_get_uint32(d,v)

Please explain. Please explain good. Really good.

> Index: common/game.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/game.c,v
> retrieving revision 1.176
> retrieving revision 1.177
> diff -u -r1.176 -r1.177
> --- common/game.c     19 Feb 2004 21:06:41 -0000      1.176
> +++ common/game.c     10 Apr 2004 03:47:49 -0000      1.177
> @@ -273,7 +273,8 @@
>    game.num_unit_types = 0;
>    game.num_impr_types = 0;
>    game.num_tech_types = 0;
> -
> + 
> +  game.nation_count = 0;

Was that a bug in the old code?

> +/*********************************************************
> + Below are the packets that control single-player mode.  
> +*********************************************************/
> +PACKET_SINGLE_WANT_HACK_REQ=108;cs,handle-per-conn,no-handle,dsend

> + UINT32 challenge;

Since an UINT32 don't provide _this_ much entropy we should have used
a more general approach: a string. This way if we add the entropy we
don't have to change the protocol.

> +PACKET_SINGLE_PLAYERLIST_REPLY=111;sc
> +  UINT8 nplayers;
> +  STRING load_filename[MAX_LEN_PACKET];
> +  STRING name[MAX_NUM_PLAYERS:nplayers][MAX_LEN_NAME]; 
> +  STRING username[MAX_NUM_PLAYERS:nplayers][MAX_LEN_NAME]; 

> +  STRING nation_name[MAX_NUM_PLAYERS:nplayers][MAX_LEN_NAME]; 

> +  STRING nation_flag[MAX_NUM_PLAYERS:nplayers][MAX_LEN_NAME]; 

Why can't the nation_flag not be extracet from the nation name?

> +  BOOL is_alive[MAX_NUM_PLAYERS:nplayers];
> +  BOOL is_ai[MAX_NUM_PLAYERS:nplayers];
> +end

> +PACKET_OPTIONS_SETTABLE_CONTROL=112;sc,handle-via-packet

> +  UINT16 nids;

Shouldn't this be named "noptions"?

> +  UINT8 ncategories;
> +  STRING category_names[256:ncategories][MAX_LEN_NAME];
> +end
> +
> +PACKET_OPTIONS_SETTABLE=113;sc
> +  UINT16 id;
> +  STRING name[MAX_LEN_NAME];
> +  STRING short_help[MAX_LEN_PACKET];
> +  STRING extra_help[MAX_LEN_PACKET];
> +  UINT8  type;                                       /* 0:bool, 1:int, 
> 2:string */
> +
> +  SINT32 val;                                        /* value for bool or 
> int */
> +  SINT32 default_val;                                /* default for bool or 
> int */
> +  SINT32 min;                                        /* min value for int */
> +  SINT32 max;                                        /* max value for int */
> +
> +  STRING strval[MAX_LEN_PACKET];             /* space for string */
> +  STRING default_strval[MAX_LEN_PACKET];     /* space for string */
> +
> +  UINT8 category;                            /* which category this is in */

IMHO this should look like:

  SINT32 int_value;
  SINT32 int_value_default;
  SINT32 int_value_min;
  SINT32 int_value_max;

  BOOL bool_value;
  BOOL bool_value_default;

  STRING string_value[MAX_LEN_PACKET];
  STRING string_value_default[MAX_LEN_PACKET];

> Index: common/shared.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/shared.h,v
> retrieving revision 1.123
> retrieving revision 1.124
> diff -u -r1.123 -r1.124
> --- common/shared.h   29 Mar 2004 18:52:54 -0000      1.123
> +++ common/shared.h   10 Apr 2004 03:47:49 -0000      1.124
> @@ -64,6 +64,7 @@
>  #define MAX_LEN_ADDR     256 /* see also MAXHOSTNAMELEN and RFC 1123 2.1 */
>  #define MAX_LEN_VET_SHORT_NAME 8
>  #define MAX_VET_LEVELS 10

> +#define MAX_LEN_PATH 4095

See C99 and FILENAME_MAX.

>  /* Use FC_INFINITY to denote that a certain event will never occur or
>     another unreachable condition. */
> Index: doc/HACKING
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/doc/HACKING,v
> retrieving revision 1.17
> retrieving revision 1.18
> diff -u -r1.17 -r1.18
> --- doc/HACKING       8 Jan 2004 11:34:45 -0000       1.17
> +++ doc/HACKING       10 Apr 2004 03:47:49 -0000      1.18
> @@ -920,6 +920,60 @@
>     "observer" or "controller".  (But observers not yet implemented.)
>     Exists in game.game_connections, and in pconn->player->connections.
>  
> +
> +===========================================================================
> +Starting a Server from Within a Client
> +===========================================================================
> +
> +After many years of complaints regarding the ease (or lack thereof) of
> +starting a game of Freeciv [start a server, input settings on a command line,
> +start a client, and connect, etc], a method has been developed for starting
> +and playing a complete game using only the client. This has been called the
> +"extended" or "new connect dialog". This is perhaps a misnomer, but there it 
> +is. This win32 client has had this feature in some form for some time. 
> +
> +It works by forking a server from within the client and then controlling that
> +server via chatline messages. The guts of the machinery to do this can be
> +found in these files:
> +
> +client/connectdlg_common.[ch]
> +client/gui-*/connectdlg.[ch]
> +common/packets.def
> +server/gamehand.[ch]
> +server/stdinhand.[ch]
> +
> +When a player starts a client, he is presents with several options: start
> +a new game, continue a saved game and connect to a networked game. For the 
> +latter option, connect_to_server() is called and login proceeeds as normal.

> +The the first two options, connectdlg_common.c:client_start_server() is 

Double "the".

> +called. Here, a server is spawned, standard input and outputs to that process
> +are closed, and then connect_to_server() is called so the client connects to 
> +that server.
> +
> +At this point everything regarding the client/server connection is as usual;
> +however, for the client to control the server, it must have access level 
> HACK,
> +so it must verify to the server that it is local (on the same machine or at
> +least has access to the same disk as the server). The procedure is:
> +
> +1. the server creates a file using gamehand.c:create_challenge_filename() and
> +   puts the name of this file in packet_server_join_reply that it sends back
> +   to the client. The name of the file is semi-random.
> +2. The client, upon receiving confirmation that it can join the server,
> +   creates a file using the name the server selected and places a random 
> number
> +   inside that file.
> +3. The client sends a packet [packet_single_want_hack_req] with that random 
> +   number back to the server.
> +4. The server upon receiving the packet [packet_single_want_hack_req], opens
> +   the file and compares the two numbers. If the file exists and the numbers
> +   are equal the server grants that client HACK access level and sends a
> +   packet [packet_single_want_hack_reply] informing the client of its 
> elevated
> +   access level.
> +

> +Only one other packet is used --- packet_single_playerlist_req --- which asks

Rephrase to "In addition to the changes to get the local client hack
access only another packet change is required --- ...."

> +the server to send a player list when a savegame is loaded. This list is used
> +for player selection.

> +/************************************************************************** 
> +find a suitably random file that we can write too, and return it's name.
> +**************************************************************************/
> +const char *create_challenge_filename(void)

This function shouldn't return the string. This is confusing.

> +/************************************************************************** 
> +opens a file specified by the packet and compares the packet values with
> +the file values. Sends an answer to the client once it's done.
> +**************************************************************************/
> +void handle_single_want_hack_req(struct connection *pc, int challenge)
> +{
> +  struct section_file file;
> +  int entropy = 0;
> +  bool could_load = TRUE;
> +  bool you_have_hack = FALSE;
> +
> +  if (section_file_load_nodup(&file, challenge_filename)) {

> +    entropy = secfile_lookup_int_default(&file, 0, "challenge.entropy");
...
> +  you_have_hack = (could_load && entropy && entropy == challenge);

Now I see why you don't like the value 0 here. But this could be changed easily.


> Index: server/srv_main.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/server/srv_main.h,v
> retrieving revision 1.17
> retrieving revision 1.18
> diff -u -r1.17 -r1.18
> --- server/srv_main.h 28 Nov 2003 17:37:22 -0000      1.17
> +++ server/srv_main.h 10 Apr 2004 03:47:50 -0000      1.18
> @@ -34,7 +34,7 @@
>    /* filenames */
>    char *log_filename;
>    char *gamelog_filename;

> -  char *load_filename;
> +  char load_filename[512]; /* FIXME: may not be long enough? use MAX_PATH? */

Use MAX_LEN_PATH.


-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "We've all heard that a million monkeys banging on a million typewriters
  will eventually reproduce the entire works of Shakespeare.
  Now, thanks to the Internet, we know this is not true."