Данило Шеган 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
_______________________________________________
Mailing list: https://launchpad.net/~vmbuilder
Post to : [email protected]
Unsubscribe : https://launchpad.net/~vmbuilder
More help : https://help.launchpad.net/ListHelp
