vmbuilder team mailing list archive
-
vmbuilder team
-
Mailing list archive
-
Message #00035
[Merge] lp:~danilo/vmbuilder/bug-536940 into lp:vmbuilder
Данило Шеган has proposed merging lp:~danilo/vmbuilder/bug-536940 into lp:vmbuilder.
Requested reviews:
VMBuilder (vmbuilder)
Related bugs:
#536940 -tmpfs=- option broken in 0.12.2-0ubuntu3
https://bugs.launchpad.net/bugs/536940
= Bug 536940 =
So, this re-introduces a --tmpfs and --tmp options. I'm not entirely sure of the syntax for --tmpfs, so I implemented what seems to have been the idea according to the documentation and old web site snippets I found. I'd probably prefer it if it was two options ("--use-tmpfs" and "--tmpfs-size"), but I don't have the time to implement that.
I've also simplified util.tmpfile and util.tmpdir functions: they were always called with the same value of 'keep' parameter. I'd be either leaning on removing them altogether, or making them useful for something (like providing a standard vmbuilder prefix like "vmbuilder" :): they are now single-line calls to tempfile module, and we should just use it directly then.
I've done a few more cleanups of my own code as well since I first written it up: I've removed catching of VMBuilderUserError exception because a run_cmd exception will contain much more useful information than my error message in util.set_up_tmpfs and util.clean_up_tmpfs.
There is also one thing I am unsure about: I'm unmounting a tmpfs even when an error occurs, but that means that you can't go into a chroot and figure out what went wrong. I found that more useful (and when I did hit a problem that needed me to go into chroot directly, and wasn't just an error on my part, I simply ran it without --tmpfs option), but I can easily be convinced the opposite.
FWIW, that sys.exit(1) call is removed simply because it's never executed with a raise just before it.
Note: I won't have much time to work on this. For instance, to actually make tmpfile calls safer, we'd at least do the following: actually create a file using tempfile.mkstemp(), and remove it only just prior to doing the operation which is going to write to that file. That wouldn't remove the risks, would just make them much, much smaller. Of course, I understand that vmbuilder is not designed to be run on machines with untrusted users, so even if I had time to spend on it, I don't think it'd be worthwhile.
--
https://code.launchpad.net/~danilo/vmbuilder/bug-536940/+merge/27290
Your team VMBuilder is requested to review the proposed merge of lp:~danilo/vmbuilder/bug-536940 into lp:vmbuilder.
=== modified file 'VMBuilder/contrib/cli.py'
--- VMBuilder/contrib/cli.py 2010-04-07 19:47:58 +0000
+++ VMBuilder/contrib/cli.py 2010-06-10 17:22:23 +0000
@@ -22,6 +22,7 @@
import pwd
import shutil
import sys
+import tempfile
import VMBuilder
import VMBuilder.util as util
from VMBuilder.disk import parse_size
@@ -32,6 +33,7 @@
arg = 'cli'
def main(self):
+ tmpfs_mount_point = None
try:
optparser = optparse.OptionParser()
@@ -49,6 +51,19 @@
group.add_option('--destdir', '-d', type='str', help='Destination directory')
group.add_option('--only-chroot', action='store_true', help="Only build the chroot. Don't install it on disk images or anything.")
group.add_option('--existing-chroot', help="Use existing chroot.")
+ group.add_option(
+ '--tmp', '-t', metavar='DIR', dest='tmp_root',
+ default=tempfile.gettempdir(),
+ help=(
+ 'Use TMP as temporary working space for image generation. '
+ 'Defaults to $TMPDIR if it is defined or /tmp otherwise. '
+ '[default: %default]'))
+ group.add_option(
+ '--tmpfs', metavar="SIZE",
+ help=(
+ 'Use a tmpfs as the working directory, specifying '
+ 'its size or "-" to use tmpfs default '
+ '(suid,dev,size=1G).'))
optparser.add_option_group(group)
group = optparse.OptionGroup(optparser, 'Disk')
@@ -105,7 +120,16 @@
distro.set_chroot_dir(self.options.existing_chroot)
distro.call_hooks('preflight_check')
else:
- chroot_dir = util.tmpdir()
+ if self.options.tmpfs is not None:
+ if str(self.options.tmpfs) == '-':
+ tmpfs_size = 1024
+ else:
+ tmpfs_size = int(self.options.tmpfs)
+ tmpfs_mount_point = util.set_up_tmpfs(
+ tmp_root=self.options.tmp_root, size=tmpfs_size)
+ chroot_dir = tmpfs_mount_point
+ else:
+ chroot_dir = util.tmpdir(tmp_root=self.options.tmp_root)
distro.set_chroot_dir(chroot_dir)
distro.build_chroot()
@@ -123,12 +147,14 @@
# and if we reach here, it means the user didn't pass
# --only-chroot. Hence, we need to remove it to clean
# up after ourselves.
- if chroot_dir:
+ if chroot_dir is not None:
util.run_cmd('rm', '-rf', '--one-file-system', chroot_dir)
except VMBuilderException, e:
logging.error(e)
raise
- sys.exit(1)
+ finally:
+ if tmpfs_mount_point is not None:
+ util.clean_up_tmpfs(tmpfs_mount_point)
def fix_ownership(self, filename):
"""
@@ -195,19 +221,19 @@
swapsize = parse_size(self.options.swapsize)
optsize = parse_size(self.options.optsize)
if hypervisor.preferred_storage == VMBuilder.hypervisor.STORAGE_FS_IMAGE:
- tmpfile = util.tmpfile(keep=False)
+ tmpfile = util.tmp_filename(tmp_root=self.options.tmp_root)
hypervisor.add_filesystem(filename=tmpfile, size='%dM' % rootsize, type='ext3', mntpnt='/')
- tmpfile = util.tmpfile(keep=False)
+ tmpfile = util.tmp_filename(tmp_root=self.options.tmp_root)
hypervisor.add_filesystem(filename=tmpfile, size='%dM' % swapsize, type='swap', mntpnt=None)
if optsize > 0:
- tmpfile = util.tmpfile(keep=False)
+ tmpfile = util.tmp_filename(tmp_root=self.options.tmp_root)
hypervisor.add_filesystem(filename=tmpfile, size='%dM' % optsize, type='ext3', mntpnt='/opt')
else:
if self.options.raw:
disk = hypervisor.add_disk(filename=self.options.raw)
else:
size = rootsize + swapsize + optsize
- tmpfile = util.tmpfile(keep=False)
+ tmpfile = util.tmp_filename(tmp_root=self.options.tmp_root)
disk = hypervisor.add_disk(tmpfile, size='%dM' % size)
offset = 0
disk.add_part(offset, rootsize, default_filesystem, '/')
@@ -222,7 +248,8 @@
try:
for line in file(self.options.part):
elements = line.strip().split(' ')
- tmpfile = util.tmpfile(keep=False)
+ tmpfile = util.tmp_filename(
+ tmp_root=self.options.tmp_root)
if elements[0] == 'root':
hypervisor.add_filesystem(elements[1], default_filesystem, filename=tmpfile, mntpnt='/')
elif elements[0] == 'swap':
@@ -256,10 +283,12 @@
except IOError, (errno, strerror):
hypervisor.optparser.error("%s parsing --part option: %s" % (errno, strerror))
-
+
def do_disk(self, hypervisor, curdisk, size):
default_filesystem = hypervisor.distro.preferred_filesystem()
- disk = hypervisor.add_disk(util.tmpfile(keep=False), size+1)
+ disk = hypervisor.add_disk(
+ util.tmp_filename(tmp_root=self.options.tmp_root),
+ size+1)
logging.debug("do_disk - size: %d" % size)
offset = 0
for pair in curdisk:
=== modified file 'VMBuilder/util.py'
--- VMBuilder/util.py 2010-02-25 23:41:18 +0000
+++ VMBuilder/util.py 2010-06-10 17:22:23 +0000
@@ -168,18 +168,36 @@
logging.debug('No such method')
return
-def tmpfile(suffix='', keep=True):
- (fd, filename) = tempfile.mkstemp(suffix=suffix)
- os.close(fd)
- if not keep:
- os.unlink(filename)
- return filename
-
-def tmpdir(suffix='', keep=True):
- dir = tempfile.mkdtemp(suffix=suffix)
- if not keep:
- os.rmdir(dir)
- return dir
+def tmp_filename(suffix='', tmp_root=None):
+ # There is a risk in using tempfile.mktemp(): it's not recommended
+ # to run vmbuilder on machines with untrusted users.
+ return tempfile.mktemp(suffix=suffix, dir=tmp_root)
+
+def tmpdir(suffix='', tmp_root=None):
+ return tempfile.mkdtemp(suffix=suffix, dir=tmp_root)
+
+def set_up_tmpfs(tmp_root=None, size=1024):
+ """Sets up a tmpfs storage under `tmp_root` with the size of `size` MB.
+
+ `tmp_root` defaults to tempfile.gettempdir().
+ """
+ mount_point = tmpdir('tmpfs', tmp_root)
+ mount_cmd = ["mount", "-t", "tmpfs",
+ "-o", "size=%dM,mode=0770" % int(size),
+ "tmpfs", mount_point ]
+ logging.info('Mounting tmpfs under %s' % mount_point)
+ logging.debug('Executing: %s' % mount_cmd)
+ run_cmd(*mount_cmd)
+
+ return mount_point
+
+def clean_up_tmpfs(mount_point):
+ """Unmounts a tmpfs storage under `mount_point`."""
+ umount_cmd = ["umount", "-t", "tmpfs", mount_point ]
+ logging.info('Unmounting tmpfs from %s' % mount_point)
+ logging.debug('Executing: %s' % umount_cmd)
+ run_cmd(*umount_cmd)
+
def get_conf_value(context, confparser, key):
confvalue = None
