Re: [Nbd] [PATCH 4/4] nbd-server: remove global variables: modern_listen and modernport

[Tuomas Jorma Juhani Räsänen <tuomasjjrasanen@...1261...>]

On Fri, Jan 04, 2013 at 02:40:59AM +0100, Wouter Verhelst wrote:
> On Thu, Jan 03, 2013 at 11:21:16PM +0200, Tuomas Jorma Juhani Räsänen wrote:
> > Address and port of the modern socket are read from the main
> > configuration file and used only when opening the socket. There is no
> > need to keep them hanging in the global scope.
> > 
> > Signed-off-by: Tuomas Jorma Juhani Räsänen <tuomasjjrasanen@...1261...>
> > ---
> >  nbd-server.c |   18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/nbd-server.c b/nbd-server.c
> > index a2f64ea..debe881 100644
> > --- a/nbd-server.c
> > +++ b/nbd-server.c
> > @@ -182,9 +182,6 @@ int modernsock=-1;	  /**< Socket for the modern handler. Not used
> >  			       command line; only port used if
> >  			       oldstyle is set to false (and then the
> >  			       command-line client isn't used, gna gna) */
> > -char* modern_listen;	  /**< listenaddr value for modernsock */
> > -char* modernport=NBD_DEFAULT_PORT; /**< Port number on which to listen for
> > -			              new-style nbd-client connections */
> >  
> >  bool logged_oversized=false;  /**< whether we logged oversized requests already */
> >  
> > @@ -2502,7 +2499,7 @@ int setup_serve(SERVER *serve) {
> >  	}
> >  }
> >  
> > -void open_modern(void) {
> > +void open_modern(const gchar *const addr, const gchar *const port) {
> >  	struct addrinfo hints;
> >  	struct addrinfo* ai = NULL;
> >  	struct sock_flags;
> > @@ -2514,7 +2511,7 @@ void open_modern(void) {
> >  	hints.ai_socktype = SOCK_STREAM;
> >  	hints.ai_family = AF_UNSPEC;
> >  	hints.ai_protocol = IPPROTO_TCP;
> > -	e = getaddrinfo(modern_listen, modernport, &hints, &ai);
> > +	e = getaddrinfo(addr, port ? port : NBD_DEFAULT_PORT, &hints, &ai);
> 
> port 0 is used to signal inetd mode. I don't know whether that's still
> used anywhere or indeed if it even still works, but removing that
> feature might be a bit premature, so you can't use a value of zero to
> indicate 'no port was chosen by the user'.

Note that port is not an integer, it's a string (as it was previously
and probably because getaddrinfo() accepts strings. Port can be a
numeric port or service name.), and can be NULL if port was not
specified at all, in which case this just defaults to
NBD_DEFAULT_PORT. This patch does not change semantics, it just
changes the way the address and port are used to open and setup the
socket.

> 
> >  	if(e != 0) {
> >  		fprintf(stderr, "getaddrinfo failed: %s\n", gai_strerror(e));
> >  		exit(EXIT_FAILURE);
> > @@ -2543,7 +2540,8 @@ void open_modern(void) {
> >  /**
> >   * Connect our servers.
> >   **/
> > -void setup_servers(GArray* servers) {
> > +void setup_servers(GArray *const servers, const gchar *const modernaddr,
> > +                   const gchar *const modernport) {
> >  	int i;
> >  	struct sigaction sa;
> >  	int want_modern=0;
> > @@ -2552,7 +2550,7 @@ void setup_servers(GArray* servers) {
> >  		want_modern |= setup_serve(&(g_array_index(servers, SERVER, i)));
> >  	}
> >  	if(want_modern) {
> > -		open_modern();
> > +		open_modern(modernaddr, modernport);
> >  	}
> >  	children=g_hash_table_new_full(g_int_hash, g_int_equal, NULL, destroy_pid_t);
> >  
> > @@ -2701,8 +2699,6 @@ int main(int argc, char *argv[]) {
> >  	
> >          /* Update global variables with parsed values. This will be
> >           * removed once we get rid of global configuration variables. */
> > -        modern_listen = genconf.modernaddr ? genconf.modernaddr : modern_listen;
> > -        modernport    = genconf.modernport ? genconf.modernport : modernport;
> >          glob_flags   |= genconf.flags;
> >  
> >  	if(serve) {
> > @@ -2752,11 +2748,13 @@ int main(int argc, char *argv[]) {
> >  	}
> >  	if (!dontfork)
> >  		daemonize(serve);
> > -	setup_servers(servers);
> > +	setup_servers(servers, genconf.modernaddr, genconf.modernport);
> >  	dousers(genconf.user, genconf.group);
> >  
> >          g_free(genconf.user);
> >          g_free(genconf.group);
> > +        g_free(genconf.modernaddr);
> > +        g_free(genconf.modernport);
> 
> same here: why?

The same reasoning as in the previous patch: genconf.modernaddr and
genconf.modernport are allocated by g_key_file_get_string() and needs
to be freed at some point. After this point, no one cares about them
anymore, so it's safe to free them.

-- 
Tuomas