On Wed, Nov 15, 2006 at 05:45:09AM -0500, Daniel Veillard wrote:
> On Wed, Nov 15, 2006 at 02:33:26AM +0000, Daniel P. Berrange wrote:
> > This is the main patch. It adds a new 'xm config file' backend driver to
> > libvirt. This basically just scans /etc/xen for config files, reads them
> > and turns them into libvirt XML to create domains. Similarly it can accept
> > libvirt XML and create config files from it.
>
> Haha, the whole point of the exercise :-)
>
> > Some important points
> >
> > - It can override /etc/xen with LIBVIRT_XM_CONFIG_DIR env variable
I added an extra check so that this environment variable is *NOT* used
if getuid() != geteuid(), or getgid() != getegid() to avoid a potential
security issue in setuid usage.
> > - It only re-scans /etc/xen every X seconds & caches parsed config
> > files in-memory to avoid parsing / I/O overhead
> > - Automatically generates a UUID if the config file doesn;t have one
> > - Skips over a blacklist of files from /etc/xen which are known to
> > not be valid config files to avoid too many error messages
> > - Filters list of inactive domains to strip out any which are currently
> > running (acccording to XenD)
> > - Allows changing of VCPUS, memory & max memory config settings and
> > writes changes back out to disk
> > - Can start, create, delete domains from disk
>
> > + /* This method is called by various methods to scan /etc/xen
> > + (or whatever directory was set by LIBVIRT_XM_CONFIG_DIR
> > + environment variable) and process any domain configs. It
> > + has rate-limited so never rescans more frequently than
> > + once every X seconds */
>
> Right, I don't want to be chastanized publicly by Dave Jones again
> at OLS 2007 :-)
>
> > + static int xenXMConfigCacheRefresh(void) {
> [...]
> > + /* Skip anything which isn't a file (takes care of scripts/ subdir */
> > + if ((stat(path, &st) < 0) ||
> > + !S_ISREG(st.st_mode)) {
>
> I would put extra braces (!S_ISREG(st.st_mode)) ... paranoid as usual :-)
Ok, added that.
> > + memset(info, 0, sizeof(virDomainInfo));
> > + if (xenXMConfigGetInt(entry->conf, "memory", &mem) < 0 ||
> > + mem < 0)
> > + info->memory = 64 * 1024;
>
> 64 MB is a minimal memory limit that we are referencing in various places
> in libvirt for Xen back-ends, maybe that ought to be isolated in internal.h
I committed the code as is for now. There're a bunch of things which can
be shared between various backends, so I'll knock up a separate patch to
pull out some of this duplicated code.
> > + virBufferAdd(buf, "<domain type='xen'>\n", -1);
> > + virBufferVSprintf(buf, " <name>%s</name>\n", name);
>
> Okay one of the things which scares me a bit here is encoding for the
> strings. We generate XML files without any encoding information which means
> it has to be UTF-8 (or UTF-16 but clearly it's not), it was more or less
> fine to do those direct buffers write before because we were in a contained
> environment, but now those strings come from external files, and well
> a chinese person may genuinely want to use chinese name in his config file
> and use BIG5 encoding for example, and in such case the config file generated
> here would not be XML. I assume config files generated by virt-manager
> when creating new domains are likely to use the strings returned from
> Gnome widget, I wonder if it is UTF-8 or the encoding of the user's locale.
GNOME will use whatever your localized character set is defined to. Fedora
defaults to UTF-8 everywhere, but this isn't true in the general case. This
should be fixed in the virtinst library when it generates its XML to create
new domains, ensuring a sensible encoding is specified.
> I guess the simplest and cleanest at this point would be to make sure
> anything parsed by conf.[ch] is actually UTF-8, and maybe double check
> virt-manager string generation.
I think we have to expect the char set to be bsaed on user's locale rather
than assuming the config files are UTF-8 ? This all said, the only bit of
the config file which is free text is its name, and since the name is used
for the filename of the config too, this will have to be encoded UTF-8. So
I guess validating UTF-8 compliance when parsing is sufficient.
> > + virBufferVSprintf(buf, " <graphics type='vnc' port='%d'/>\n", (5900+display));
>
> Shouldn't the 5900 magic number be in a header too ?
Yes, will refactor later.
> Good to finally have the bridge from config files to libvirt loading :-)
>
> Very cool ! Push it :-)
Committed.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|