Daniel Stekloff
2005-Sep-22 20:14 UTC
[Xen-devel] [PATCH] xend - Have parseConfig check configuration options
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. I will look at making the options consistent, if this is agreeable. Signed-off-by: Daniel Stekloff <dsteklof@us.ibm.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ewan Mellor
2005-Sep-24 19:25 UTC
Re: [Xen-devel] [PATCH] xend - Have parseConfig check configuration options
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 intovalidateInfo -- 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@us.ibm.com> > > >> 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@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel