On Wed, Mar 11, 2009 at 05:42:20PM +0100, Florian Vichot wrote:
> Hi everyone,
>
> This patch is to allow using the "mount" type in the "filesystem" tag
> for OpenVZ domains.
>
> Example:
> ...
> <filesystem type='mount'>
> <source dir='/path/to/filesystem/directory/' />
> <target dir='/path/to/pivot/root/' />
> </filesystem>
> ...
>
> This is my first patch to an external project, so don't spare me if I
> got things wrong :)
>
> Also, I'm curious for suggestions as to how I could allow for the target
> not to be specified in the XML. Because in this case OpenVZ just makes a
> temporary pivot root in "/var/lib/vz/root/" and that is probably
> sufficient for most people, who might not want to have to explicitly
> create a pivot root somewhere, just for mounting the filesystem while
> it's running.
Actually the <target dir="..."> means something a little different. This
refers to the mount point *within* the container. So for the root filesystem
of the container it will be <target dir='/' />.
I believe OpenVZ only allows you to setup the root filesystem within the
container, so effectively you'd never need worry about anything other
than the one <target dir='/' />. IMHO there is no need to include the
temporary pivot root location in the XML, just leave it on default.
As an example, of why we have this, the LXC container driver, allows for
specifying multiple filesystems for containers. So you could setup lots
of containers each with their own private root fileystem, and then give
them all the same /home,
eg, in guest 1 you'd have
<filesystem type='mount'>
<source dir='/path/to/root/filesystem/guest1/' />
<target dir='/' />
</filesystem>
<filesystem type='mount'>
<source dir='/path/to/shared/home' />
<target dir='/home' />
</filesystem>
While in guest 2 you'd have
<filesystem type='mount'>
<source dir='/path/to/root/filesystem/guest2/' />
<target dir='/' />
</filesystem>
<filesystem type='mount'>
<source dir='/path/to/shared/home' />
<target dir='/home' />
</filesystem>
> diff --git a/src/openvz_conf.c b/src/openvz_conf.c
> index ff3d607..33fb259 100644
> --- a/src/openvz_conf.c
> +++ b/src/openvz_conf.c
> @@ -314,6 +314,41 @@ error:
> }
>
>
> +/* utility function to replace 'from' by 'to' in 'str' */
> +static char*
> +openvz_replace(const char* str,
> + const char* from,
> + int to) {
> + char tmp[4096];
> + char* offset = tmp;
> + int len = strlen(str);
> + int fromLen = strlen(from);
> + int r,i,maxcmp,left = sizeof(tmp);
> +
> + if(!from)
> + return NULL;
> +
> + for(i = 0; i < len && left; ++i) {
> + /* compare first caracters to limit useless full comparisons */
> + if(*from == str[i]) {
> + maxcmp = (fromLen > len-i) ? len-i : fromLen;
> + if(strncmp(from, str+i, maxcmp) == 0) {
> + r = snprintf(offset, left, "%d", to);
> + offset += r;
> + i += fromLen - 1;
> + continue;
> + }
> + }
> + *offset++ = str[i];
> + left = sizeof(tmp) - (offset-tmp);
> + }
> +
> + tmp[sizeof(tmp)-left] = '\0';
> +
> + return strdup(tmp);
> +}
In this function, can you use STREQLEN(from, str+i, maxcmp) instead
of strncmp == 0.
Also, I think it'd be clearer to avoid using the pre-declared 'char tmp[4096]'
and instead make use of our automatic grow-on-demand string buffer code.
Just add
#include "buf.h"
Declare it with:
virBuffer buf = VIR_BUFFER_INITIALIZER;
And then use virBufferAdd to append to it
When you're all done, use virBufferError() to check if there was an OOM
condition and return an error. If OK,then use virBufferContentAndReset()
to get the internal char * - no need to strdup() that - you own the
pointer at that point.
> @@ -343,7 +378,42 @@ openvzReadFSConf(virConnectPtr conn,
> goto no_memory;
> def->fss[def->nfss++] = fs;
> fs = NULL;
> + } else {
> + /* OSTEMPLATE was not found, VE was booted from a private dir directly */
> + ret = openvzReadConfigParam(veid, "VE_PRIVATE", temp, sizeof(temp));
> + if (ret <= 0) {
> + openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> + _("Cound not read 'VE_PRIVATE' from config for container %d"),
> + veid);
> + goto error;
> + }
> +
> + if (VIR_ALLOC(fs) < 0)
> + goto no_memory;
> +
> + fs->type = VIR_DOMAIN_FS_TYPE_MOUNT;
> + fs->src = openvz_replace(temp, "$VEID", veid);
> +
> + if (fs->src == NULL)
> + goto no_memory;
> +
> + ret = openvzReadConfigParam(veid, "VE_ROOT", temp, sizeof(temp));
> + if (ret <= 0) {
> + openvzError(conn, VIR_ERR_INTERNAL_ERROR,
> + _("Cound not read 'VE_ROOT' from config for container %d"),
> + veid);
> + goto error;
> + }
> +
> + fs->dst = openvz_replace(temp, "$VEID", veid);
>
> + if (fs->dst == NULL)
> + goto no_memory;
> +
> + if (VIR_REALLOC_N(def->fss, def->nfss + 1) < 0)
> + goto no_memory;
> + def->fss[def->nfss++] = fs;
> + fs = NULL;
> }
>
> return 0;
> @@ -597,6 +667,81 @@ openvzReadConfigParam(int vpsid ,const char * param, char *value, int maxlen)
> return ret ;
> }
>
> +static int
> +openvz_copyfile(char* from_path, char* to_path)
> +{
> + char line[PATH_MAX];
> + int fd, copy_fd;
> + int bytes_read;
> +
> + fd = open(from_path, O_RDONLY);
> + if (fd == -1)
> + return -1;
> + copy_fd = open(to_path, O_WRONLY | O_CREAT | O_TRUNC, 0644);
> + if (copy_fd == -1) {
> + close(fd);
> + return -1;
> + }
> +
> + while(1) {
> + if (openvz_readline(fd, line, sizeof(line)) <= 0)
> + break;
> +
> + bytes_read = strlen(line);
> + if (safewrite(copy_fd, line, bytes_read) != bytes_read)
> + goto error;
> + }
> +
> + if (close(fd) < 0)
> + goto error;
> + fd = -1;
> + if (close(copy_fd) < 0)
> + goto error;
> + copy_fd = -1;
> +
> + return 0;
> +
> +error:
> + if (fd != -1)
> + close(fd);
> + if (copy_fd != -1)
> + close(copy_fd);
> + return -1;
> +}
> +
> +/*
> +* Copy the default config to the VE conf file
> +* return: -1 - error
> +* 0 - OK
> +*/
> +int
> +openvzCopyDefaultConfig(int vpsid)
> +{
> + char * confdir;
> + char default_conf_file[PATH_MAX], conf_file[PATH_MAX];
> +
> + confdir = openvzLocateConfDir();
> + if (confdir == NULL)
> + return -1;
> +
> + if (snprintf(default_conf_file, PATH_MAX, "%s/%s",
> + confdir, VZ_DEFAULT_CONF_FILE) >= PATH_MAX)
> + {
> + VIR_FREE(confdir);
> + return -1;
> + }
Could you make use of virAsprintf() here instead of statically declaring
a variable with PATH_MAX. I know we use PATH_MXA all over the place in
libvirt already, but we need to try to get out of that habit where
practical, to avoid wasting too much stack.
> +
> + VIR_FREE(confdir);
> +
> + if (openvzLocateConfFile(vpsid, conf_file, PATH_MAX, "conf")<0)
> + return -1;
> +
> + if (openvz_copyfile(default_conf_file, conf_file)<0)
> + return -1;
> +
> + return 0;
> +}
> +
> /* Locate config file of container
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list