This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

To: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH]: xl: introduce xlu_cfg_get_(string|long)_default
From: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
Date: Tue, 31 Aug 2010 17:22:22 +0100
Cc: Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Tue, 31 Aug 2010 09:26:06 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1283271142.12544.9446.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1283269353.25936.9.camel@xxxxxxxxxxxxxxxxxxxxxx> <1283271142.12544.9446.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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?


Xen-devel mailing list