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

["Mike Kaufman" <kaufman@xxxxxxxxxxxxxxxxxxxxxx>]

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

> On Fri, Apr 09, 2004 at 09:00:42PM -0700, Raimar Falke wrote:
> 
> 
> > +         a series of packet_generic_message packets with commands to start 
> > +         the game.
> > +**************************************************************************/
> >  
> 
> There is no packet_generic_message.

well, yes, this was written before delta went in...

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

sounds fine by me.

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

sure.

> > +    /* avoid terminal spam, but still make server output available */ 
> 
> add "by redirecting the output to the logfile".

another comment written b/f the logfile came into being. fine.

> > +    /* 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?
 
Daniel wrote this part.

> > +    _exit(1);
> 
> > +  /* don't need these anymore */ 
> 
> Add "don't free the terminating NULL".

sure.

> > +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.
 
possibly. devise a method and tell me.

> > +void really_close_connection_dialog(void);
> 
> The naming is very poor. The documentation don't make it any clearer
> what this function should do.

umm. did you read the function header? Maybe we should add a "no matter
what." ?

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

if you like.
 
> > +  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.

probably a good idea.

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

what upper bound? 65535?

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

we had this discussion already. the answer is no.
> 
> > 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.

it's 'explain well'... 
it's simple really. If you'd actually written a function instead of just
the function prototype, I'd have used that.

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

coulda. still can if you want to change the protocol now.

> > +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?

no. the rulesets haven't been transferred over yet. this really is a 'bug'
that needs to be fixed.

> > +PACKET_OPTIONS_SETTABLE_CONTROL=112;sc,handle-via-packet
> 
> > +  UINT16 nids;
> 
> Shouldn't this be named "noptions"?

maybe. you wanted it to be nids though IIRC. at least you disliked nopts.

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

I know you do. no.

> > +#define MAX_LEN_PATH 4095
> 
> See C99 and FILENAME_MAX.

I'm too lazy.
 
> > +The the first two options, connectdlg_common.c:client_start_server() is 
> 
> Double "the".

ur.

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

hence the FIXME. The only problem being that this is probably bigger than
MAX_LEN_PACKET...

you want to write a patch to fix these issues, I'll certainly commit it.

-mike