Re: [Nbd] [PATCH 3/4] nbd-server: remove global variables: runuser and rungroup

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

On Fri, Jan 04, 2013 at 02:39:04AM +0100, Wouter Verhelst wrote:
> On Thu, Jan 03, 2013 at 11:21:05PM +0200, Tuomas Jorma Juhani Räsänen wrote:
> > User and group names are needed only at the very beginning of the
> > process, 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 |   26 ++++++++++++--------------
> >  1 file changed, 12 insertions(+), 14 deletions(-)
> > 
> > diff --git a/nbd-server.c b/nbd-server.c
> > index 82490fe..a2f64ea 100644
> > --- a/nbd-server.c
> > +++ b/nbd-server.c
> > @@ -116,10 +116,6 @@
> >  /** Where our config file actually is */
> >  gchar* config_file_pos;
> >  
> > -/** What user we're running as */
> > -gchar* runuser=NULL;
> > -/** What group we're running as */
> > -gchar* rungroup=NULL;
> >  /** global flags */
> >  int glob_flags=0;
> >  
> > @@ -2621,24 +2617,24 @@ void daemonize(SERVER* serve) {
> >  /**
> >   * Set up user-ID and/or group-ID
> >   **/
> > -void dousers(void) {
> > +void dousers(const gchar *const username, const gchar *const groupname) {
> >  	struct passwd *pw;
> >  	struct group *gr;
> >  	gchar* str;
> > -	if(rungroup) {
> > -		gr=getgrnam(rungroup);
> > +	if (groupname) {
> > +		gr = getgrnam(groupname);
> >  		if(!gr) {
> > -			str = g_strdup_printf("Invalid group name: %s", rungroup);
> > +			str = g_strdup_printf("Invalid group name: %s", groupname);
> >  			err(str);
> >  		}
> >  		if(setgid(gr->gr_gid)<0) {
> >  			err("Could not set GID: %m"); 
> >  		}
> >  	}
> > -	if(runuser) {
> > -		pw=getpwnam(runuser);
> > +	if (username) {
> > +		pw = getpwnam(username);
> >  		if(!pw) {
> > -			str = g_strdup_printf("Invalid user name: %s", runuser);
> > +			str = g_strdup_printf("Invalid user name: %s", username);
> >  			err(str);
> >  		}
> >  		if(setuid(pw->pw_uid)<0) {
> > @@ -2705,8 +2701,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. */
> > -        runuser       = genconf.user ? genconf.user : runuser;
> > -        rungroup      = genconf.group ? genconf.group : rungroup;
> >          modern_listen = genconf.modernaddr ? genconf.modernaddr : modern_listen;
> >          modernport    = genconf.modernport ? genconf.modernport : modernport;
> >          glob_flags   |= genconf.flags;
> > @@ -2759,6 +2753,10 @@ int main(int argc, char *argv[]) {
> >  	if (!dontfork)
> >  		daemonize(serve);
> >  	setup_servers(servers);
> > -	dousers();
> > +	dousers(genconf.user, genconf.group);
> > +
> > +        g_free(genconf.user);
> > +        g_free(genconf.group);
> 
> why?

genconf.user and genconf.group are strings allocated by
g_key_file_get_string(), they need to be freed at some point, and
after this point, no one ever cares about them anymore: dousers() is
the only function interested in user and group names.

-- 
Tuomas