[Freeciv-Dev] Re: (PR#8572) ext conn dialog improvements

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

Vasco Alexandre da Silva Costa wrote:

>   * removed Win32 support code, I do not think it works, something to
> add back later.

Dont' do this.  The win32 support code doesn't fully work (gtk2 on win32 
won't have a working conndlg) but it is a work in progress - a partial 
conversion of the conndlg in gui-win32.  Also removing it will break 
gui-win32.

>  /************************************************************************** 
> -Kills the server if the client has started it (FIXME: atexit handler?)
> +  Kills the server if the client has started it (FIXME: atexit handler?)
>  **************************************************************************/ 
> -void client_kill_server()
> +void client_kill_server(void)
>  {
>    if (is_server_running()) {
> -#ifdef WIN32_NATIVE
> -    TerminateProcess(server_process, 0);
> -    CloseHandle(server_process);
> -    server_process = INVALID_HANDLE_VALUE;           
> -#else
>      kill(server_pid, SIGTERM);
> -    waitpid(server_pid, NULL, WUNTRACED);
> -    server_pid = - 1;
> -#endif    
>    }
>  }   

I think this is in error also.  The waitpid and server_pid=-1 lines 
should stay.  I think this function actually needs to block waiting for 
the server to exit.

Or do I misunderstand the use of child_reaper?

> +/************************************************************************** 
> +...
> +**************************************************************************/ 
> +const char *server_paths[] = {
> +  "./ser",
> +  "./server/civserver",
> +  "civserver"
> +};
> +
> +/************************************************************************** 
> +  Locate server executable path.
> +**************************************************************************/ 
> +static const char *find_server_executable(void)
> +{
> +  int i;
> +
> +  for (i = 0; i < ARRAY_SIZE(server_paths); i++) {
> +    struct stat buf;
> +
> +    if (stat(server_paths[i], &buf) == 0) {
> +      return server_paths[i];
> +    }
> +  }
> +  return NULL;
> +}

Will stat("civserver") work?  I suspect it will not take $PATH into 
account which is obviously the intent.  Perhaps this should be 
"./civserver" instead?

> +  argv = fc_malloc(nargs * sizeof(*argv));
> +  argv[i++] = mystrdup("civserver");

This should be the find_server_executable string, shouldn't it?

> +  argv[i++] = mystrdup("-p");
> +  my_snprintf(buf, sizeof(buf), "%d", server_port);
> +  argv[i++] = mystrdup(buf);
>  
>    if (logfile) {
> -    argv[i] = fc_malloc(8);
> -    my_snprintf(argv[i++], 8, "--debug");
> -    argv[i] = fc_malloc(2);
> -    my_snprintf(argv[i++], 2, "3");
> -    argv[i] = fc_malloc(6);
> -    my_snprintf(argv[i++], 6, "--log");
> -    argv[i] = fc_malloc(strlen(logfile) + 1);
> -    my_snprintf(argv[i++], strlen(logfile) + 1, logfile);
> +    argv[i++] = mystrdup("--debug");
> +    argv[i++] = mystrdup("3");
> +    argv[i++] = mystrdup("--log");
> +    argv[i++] = mystrdup(logfile);
>    }

Why do these need to be strdup'd at all?

> +  /* Set up callbacks */
> +  if (!callbacks_setup) {
> +    signal(SIGCHLD, reaper);
> +    atexit(client_kill_server);
> +    callbacks_setup = TRUE;
> +  }

Is this portable?

> -    fd = open("/dev/null", O_RDONLY);
> -    if (fd != 0) {
> +    if ((fd = open("/dev/null", O_RDONLY)) != 0) {

This, and some (just a few) of the comment changes, seem like they're 
just noise.  Maybe someone else will just make a patch to change them 
back...

> -#else /* Can't do much without fork(). */
> +#else /* Can't do much without fork() */

Yeah, like this one.

No comment on the GUI changes.

These are good changes, but we probably want to do them one at a time so 
we can inspect them closer.  This is pretty senstive code.

jason