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
------------------------------------------------------------------------------
Master HTML5, CSS3, ASP.NET, MVC, AJAX, Knockout.js, Web API and
much more. Get web development skills now with LearnDevNow -
350+ hours of step-by-step video tutorials by Microsoft MVPs and experts.
SALE $99.99 this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122812
_______________________________________________
Nbd-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/nbd-general
