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

[Justin Moore <justin@xxxxxxxxxxx>]

Overall this looks really good.  Much much nicer to start games up. :)

> > - 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.
>
> I don't really think there is.
>
> For the server to tell the client when it's ready through the network
> would require the client to use server-like network features and ensure
> general ugliness.  For it to do so over the pipe would mean ugly,
> bug-prone and CPU-intesive busy-waiting.  For the client to just keep
> trying makes more sense.

A few comments/ideas:
- Cap the number of ports the server will try to bind to.  do/while loops
    with problematic terminating conditions are bad.  Maybe give up after
    a thousand (or 1024) attempts?  We'd want to avoid a DoS.
- Use an exponential backoff for the client attempting to connect to the
    server.  Give up after about 30 seconds.
- Have the server dump its pid to a well-known file ($HOME/.civserverpid
    or the like) after it's ready to accept connections.  The client can
    try to stat() that file and make sure the PID contained within is the
    same as the one it got from fork().  This could be used along with the
    exponential backoff, and for a more accurate is_server_running.

> > 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).
> >
>
> Since the server controls are based on being on owning the server, I don't
> really see the need to use the network (except where it's
> easier).  If/when we get real server controls in the client, the start and
> score buttons will become redundant, but there will still be a need for
> extra controls for the server owner.

   I agree with the previous poster (Jason?) about using the network.  For
the sake of being transparent, any future GUI improvements should
communicate over the socket and somehow parse the response.  I don't fully
remember the thread, but wasn't there a discussion about having more
information stored on a per-connection basis (compressed I/O, rfcstyle
versus the binary one, etc)?

> > - 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.
> >
>
> They're pretty thorough, and now that failure gives an error message,
> they're probably adequate.  If you've got a better idea, I'd be happy to
> change them, though.

  I would suggest also adding "../server/civserver".

One or two other things:
- send_server_commandline could use strchr to see if the command is already
    newline-terminated or if it needs one; also check to make sure there's
    actually something to be sent ('s' is len > 0)
- Any comments on using fdopen to do line buffering around the server
    socket?  I'm not 100% sure how errors are handled (i.e., the server
    dies, what does fgets return?) but it may be easier than doing
    essentially the same thing (since there's little or no error checking
    in there right now).

Overall, great patch. :)

-jdm

Department of Computer Science, Duke University, Durham, NC 27708-0129
Email:  justin@xxxxxxxxxxx