Re: [Xen-devel] [PATCH] xend - Have parseConfig check configuration options

[Ewan Mellor <ewan@xxxxxxxxxxxxx>]

On Thu, Sep 22, 2005 at 01:14:44PM -0700, Daniel Stekloff wrote:

> 
> This patch adds a check in parseConfig() to see if certain config
> options are valid, it sets defaults if they aren't. I realize that some
> of these defaults are set by xm - like "name". I think, however, it's a
> good idea for xend to check too.
> 
> The recent bug 246 was solved first by putting a check in initDomain to
> see if "cpu" was None and then by only using "cpu" if there's a value.
> 
> http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=246
> 
> Currently, the configuration options are dealt with in xm and then in
> different areas within xend. Shouldn't xend check for options and assign
> default values in one spot for easy maintenance? Shouldn't it check when
> it runs parseConfig? 
> 
> Also, it's possible for get_cfg(), which is in parseConfig(), to return
> None for some options that aren't defined in the config file - like
> "cpu". The "if conv and not val is None:" check won't hit val == None
> and then get_cfg() returns val. 

Hi Daniel,

Configuration options are already checked -- they just aren't checked inside
parseConfig, but inside validateInfo instead.  The intention here is to
separate the validation from the configuration parsing, which then means that
we can apply the validation to values from the hypervisor just as easily as we
can apply it to values from the configuration file.

It is reasonable for us to use None to mean 'no value specified', which is
precisely what happens inside parseConfig.  This can then either be rejected
by validateInfo, or a reasonable default substituted, or None allowed to
propagate if that is appropriate.  In the case of bug #246, the value None was
allowed to propagate, as it is reasonable for no cpu to be specified.  In this
case, this means "please don't pin this domain to any particular CPU".

You could argue that -1 is a reasonable value for "please don't pin".
However, that means always inventing an appropriate 'unspecified' value for
each different data type -- maybe the empty string means unspecified name, or
the empty list means an unspecified CPU map, or whatever.  Python has None, so
we may as well use it -- None means unspecified, wherever it occurs.

>From your patch, I have moved the defaulting of ssidref to 0 into
validateInfo -- as far as I know, every domain must have a ssidref specified.
However, I see no value in specifying a default memory value of 128; this
value is no more reasonable than 64 or 256 or any other value.  Leaving it
unspecified is more appropriate.

Cheers,

Ewan.


> I will look at making the options consistent, if this is agreeable. 
> 
> Signed-off-by: Daniel Stekloff <dsteklof@xxxxxxxxxx>
> 
> 
> 

> diff -r 2f83ff9f6bd2 tools/python/xen/xend/XendDomainInfo.py
> --- a/tools/python/xen/xend/XendDomainInfo.py Thu Sep 22 17:03:16 2005
> +++ b/tools/python/xen/xend/XendDomainInfo.py Thu Sep 22 13:03:39 2005
> @@ -225,6 +225,18 @@
>          result['cpu_weight']   = get_cfg('cpu_weight', float)
>          result['bootloader']   = get_cfg('bootloader')
>          result['restart_mode'] = get_cfg('restart')
> + 
> +        if not result['name']:
> +            raise XendError("Invalid configuration: domain name is 
> undefined.")
> +
> +        if not result['ssidref']:
> +            result['ssidref'] = 0
> +
> +        if not result['memory']:
> +            result['memory'] = 128
> +
> +        if not result['cpu']:
> +            result['cpu'] = -1
>  
>          try:
>              imagecfg = get_cfg('image')

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


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