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

[Wouter Verhelst <w@...112...>]

On Fri, Jan 04, 2013 at 03:10:09PM +0200, Tuomas Jorma Juhani Räsänen wrote:
> 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

Ah, right. Heh. Okay, disregard that then :-)

(that'll teach me for reviewing patches at 2:40 AM...)

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

Yes, that's sensible then.

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

Mmm. My point (in both cases) was more that just because you don't need
something anymore shouldn't mean you need to also throw it out. For
instance, if we ever want to go and do a second parsing of the config
file (which is what you're trying to do here, right?) then it may make
sense to be able to error out on "the username was changed". It's hard
to do that if you don't have the user name anymore because you already
free()d the string.

Yes, technically it's a leak if you don't release memory until program
end, but I've never considered one-time leaks a problem; due to
fragmentation, you most likely won't be reusing that memory anyway, so
there's not much of a benefit here; and if we have a non-NULL pointer in
the "user we're running as" variable, then we can use that fact to
signal whether we did in fact use that option at some later point in the
program, should we ever need to.

If your concern is that we'll need to have it freed when rereading the
config file, it's not too late to free it after comparing the old values
with the new ones...

Right?

-- 
Copyshops should do vouchers. So that next time some bureaucracy requires you
to mail a form in triplicate, you can mail it just once, add a voucher, and
save on postage.