Re: [Nbd] [PATCH 2/4] nbd-server: fix parse_cfile() to not modify any global variables

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

On Fri, Jan 04, 2013 at 02:37:49AM +0100, Wouter Verhelst wrote:
> Hi Tuomas,
> 

Hi

> On Thu, Jan 03, 2013 at 11:20:53PM +0200, Tuomas Jorma Juhani Räsänen wrote:
> > This change allows parse_cfile() to be called without any side-effects
> > and passes the decision of what to do with parsed results to the
> > caller.
> 
> Right, that's the big one :-)

Unfortunately :(

I hate big ones. I so hate them.

> 
> [...]
> > -	if(i==1 && have_global) {
> > +	if(i==1 && genconf) {
> >  		g_set_error(e, errdomain, CFILE_NO_EXPORTS, "The config file does not specify any exports");
> >  	}
> > +
> > +        if (genconf) {
> > +                /* Return the updated generic configuration through the
> > +                 * pointer parameter. */
> > +                memcpy(genconf, &genconftmp, sizeof(struct generic_conf));
> > +        } else {
> > +                /* The caller is not interested in the generic
> > +                 * configuration, discard everything we parsed. */
> > +                g_free(genconftmp.user);
> > +                g_free(genconftmp.group);
> > +                g_free(genconftmp.modernaddr);
> > +                g_free(genconftmp.modernport);
> > +        }
> 
> This seems wrong.
> 
> Maybe the "have_global" parameter was misnamed, but it's meant to be set
> to FALSE when we're parsing a config file from an "includedir"
> statement. That is, it calls itself recursively, and thus can be called
> in two ways:
> - Either it's called on the main config file,
>   $sysconfdir/nbd-server/config, which is _required_ to have a section
>   [generic]
> - Or it's called on a config file _snippet_ from the directory specified
>   in includedir; these are supposed to only define one export, and
>   _cannot_ have a section [generic].
> 
> In other words, if you have something to throw away there, that would
> mean you did something wrong.

You are right. I got a bit confused there. My thought was that because
g_key_file_get_string() returns an allocated string, it needs to be
freed at some point, and if the caller of the parse_cfile() wasn't
interested in those values (NULL was passed to parse_cfile()), then
parse_cfile() should free the values retrieved via
g_key_file_get_string(). But, like you said, there isn't anything to
free, because values from the generic section are not retrieved
becayse genconf==NULL.

So yes, this is wrong because it's pointless to free NULL pointers.

I'll remove the else-block and re-send the patch.

-- 
Tuomas