[Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Tue, 2010-08-31 at 17:12 +0100, Ian Campbell wrote:
> On Tue, 2010-08-31 at 16:42 +0100, Gianni Tedesco wrote:
> > The function init_dm_info() in xl_cmdimpl.c is used to initialise a
> > libxl_device_model_info structure with some default values. After being
> > called, some of those values are overridden. For the string values which
> > are not overwritten we either end up with a double free or attempt to
> > free a string literal resulting in a segfault. This type of usage model
> > would be complex to fix by adding strdup()'s in the init function and
> > then checking and freeing when over-writing.
> > 
> > My proposed solution is to add default versions of xlu_cfg_get_string
> > and xlu_cfg_get_long.
> 
> I like the idea but is it not possible to implement these as wrappers
> around the non-default providing versions and therefore avoid
> duplicating the code? (or maybe the other way round, defining the
> non-default variants in terms of default==NULL etc).

see discussion below

> >          /* then process config related to dm */
> > -        if (!xlu_cfg_get_string (config, "device_model", &buf))
> > +        if (!xlu_cfg_get_string_default (config, "device_model", &buf, 
> > "qemu-dm"))
> >              dm_info->device_model = strdup(buf);
> 
> Hasn't buf already been strdupped by xlu_cfg_get_string_default if the
> default ends up being used?
> 
> I'm not sure about set->values[0] in the other case but presumably it is
> not already dupped or we wouldn't already be doing it again. In which
> case it looks like xlu_cfg_get_string_default should return the literal
> undup'd default and let the caller take care of dupping it.
> 
> Presumably this is the same in the other cases too.

Yes that is broken, it leaks. Will fix and re-send.

> +int xlu_cfg_get_long_default(const XLU_Config *cfg, const char *n,
> > +                     long *value_r, long def) {
> > +    long l;
> > +    XLU_ConfigSetting *set;
> > +    int e;
> > +    char *ep;
> > +
> > +    e= find_atom(cfg,n,&set);  if (e) { *value_r = def; return 0; }
> > +    errno= 0; l= strtol(set->values[0], &ep, 0);
> > +    e= errno;
> > +    if (errno) {
> > +        e= errno;
> > +        assert(e==EINVAL || e==ERANGE);
> > +        fprintf(cfg->report,
> > +                "%s:%d: warning: parameter `%s' could not be parsed"
> > +                " as a number: %s\n",
> > +                cfg->filename, set->lineno, n, strerror(e));
> > +        *value_r = def;
> 
> It is unclear if the default should be used or a more serious error
> raised in the case of failure to parse the value if it is present, as
> opposed to the value not being present. I don't think you are changing
> the existing semantics though.

I implemented these semantics since it reflects current practice where
callers do not check the return value. It is also why I had duplicated
the code. Not sure exactly what to do with this...

I am all in favour of pressing on after getting some bad digits, abort()
would be too harsh, but converting all callers to distinguish between
not-present, present-and-valid and present-but-not-valid would be a lot
of extra lines of code.

Also, in xm, wouldn't a lot of these integers used as boolean be totally
valid if assigned with True/False?

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel