On Sat, Dec 28, 2013 at 5:57 PM, Brian Dolbec wrote: > Cleans up all self.mounts, self.mountmap usage. > Replace multiple path additions with one instance at the beginning of the > function, reuse the result multiple times. > Add some extra debug prints (to be converted to logging later) > --- > modules/generic_stage_target.py | 94 > ++++++++++++++++++++++++++--------------- > modules/stage1_target.py | 5 ++- > 2 files changed, 63 insertions(+), 36 deletions(-) > > diff --git a/modules/generic_stage_target.py > b/modules/generic_stage_target.py > index ce28ab8..c8d31e1 100644 > --- a/modules/generic_stage_target.py > +++ b/modules/generic_stage_target.py > @@ -4,6 +4,23 @@ from generic_target import * > from stat import * > import catalyst_lock > > +# temporary location. It will be moved to a > +# new defaults file in a later comit > +TARGET_MOUNTS_DEFAULTS = { > + "proc": "/proc", > + "dev": "/dev", > + "devpts": "/dev/pts", > + "portdir": "/usr/portage", > + "distdir": "/usr/portage/distfiles", > + "packagedir": "/usr/portage/packages", > + "port_tmpdir": "/var/tmp/portage", > + "kerncache": "/tmp/kerncache", > + "ccache": "/var/tmp/ccache", > + "icecream": "/usr/lib/icecc/bin", > + "port_logdir": "/var/log/portage", > + } > + > + > class generic_stage_target(generic_target): > """ > This class does all of the chroot setup, copying of files, etc. It > is > @@ -173,6 +190,10 @@ class generic_stage_target(generic_target): > > file_locate(self.settings,["portage_confdir"],expand=0) > > """ Setup our mount points """ > + # initialize our target mounts. > + # later I plan to make these configurable so the > + # defaults can be easily overridden if desired. > + self.target_mounts = TARGET_MOUNTS_DEFAULTS.copy() > if "SNAPCACHE" in self.settings: > self.mounts=["proc", "dev", "portdir", "distdir", > "port_tmpdir"] > self.mountmap={"proc":"/proc", "dev":"/dev", > "devpts":"/dev/pts", > @@ -219,17 +240,18 @@ class generic_stage_target(generic_target): > self.mounts.append("ccache") > self.mountmap["ccache"] = ccdir > """ for the chroot: """ > - self.env["CCACHE_DIR"]="/var/tmp/ccache" > + self.env["CCACHE_DIR"] = > self.target_mounts["ccache"] > > if "ICECREAM" in self.settings: > self.mounts.append("/var/cache/icecream") > > self.mountmap["/var/cache/icecream"]="/var/cache/icecream" > - > self.env["PATH"]="/usr/lib/icecc/bin:"+self.env["PATH"] > + self.env["PATH"] = self.target_mounts["icecream"] > + ":" + \ > + self.env["PATH"] > if "port_logdir" in self.settings: > - self.mounts.append("/var/log/portage") > - > self.mountmap["/var/log/portage"]=self.settings["port_logdir"] > - self.env["PORT_LOGDIR"]="/var/log/portage" > + self.mounts.append("port_logdir") > + > self.mountmap["port_logdir"]=self.settings["port_logdir"] > + > self.env["PORT_LOGDIR"]=self.target_mounts["port_logdir"] > self.env["PORT_LOGDIR_CLEAN"]='find > "${PORT_LOGDIR}" -type f ! -name "summary.log*" -mtime +30 -delete' > I've avoided commenting on + in strings, as my understanding was that we were not doing style fixups in these CLs. I would argue that in general, any time you use string + string in python, you are doing it wrong (e.g. your code is not 'pythonic') You can use "%s:%s" % (string1, string2) or ":".join(string1, string2) Either are fine, and are nominally better than string + string + string. > > def override_cbuild(self): > @@ -603,33 +625,34 @@ class generic_stage_target(generic_target): > "kill-chroot-pids script > failed.",env=self.env) > > def mount_safety_check(self): > - mypath=self.settings["chroot_path"] > - > """ > Check and verify that none of our paths in mypath are > mounted. We don't > want to clean up with things still mounted, and this > allows us to check. > Returns 1 on ok, 0 on "something is still mounted" case. > """ > > - if not os.path.exists(mypath): > + if not os.path.exists(self.settings["chroot_path"]): > return > > + print "self.mounts =", self.mounts > for x in self.mounts: > - if not os.path.exists(mypath + self.mountmap[x]): > + target = normpath(self.settings["chroot_path"] + > self.target_mounts[x]) > + print "mount_safety_check() x =", x, target > + if not os.path.exists(target): > continue > > - if ismount(mypath + self.mountmap[x]): > + if ismount(target): > """ Something is still mounted "" """ > try: > - print self.mountmap[x] + " is > still mounted; performing auto-bind-umount...", > + print target + " is still mounted; > performing auto-bind-umount...", > """ Try to umount stuff ourselves > """ > self.unbind() > - if ismount(mypath + > self.mountmap[x]): > - raise CatalystError, > "Auto-unbind failed for " + self.mountmap[x] > + if ismount(target): > + raise CatalystError, > "Auto-unbind failed for " + target > else: > print "Auto-unbind > successful..." > except CatalystError: > - raise CatalystError, "Unable to > auto-unbind " + self.mountmap[x] > + raise CatalystError, "Unable to > auto-unbind " + target > > def unpack(self): > unpack=True > @@ -897,12 +920,14 @@ class generic_stage_target(generic_target): > > def bind(self): > for x in self.mounts: > - if not os.path.exists(self.settings["chroot_path"] > + self.mountmap[x]): > - > os.makedirs(self.settings["chroot_path"]+x,0755) > + #print "bind(); x =", x > + target = normpath(self.settings["chroot_path"] + > self.target_mounts[x]) > + if not os.path.exists(target): > + os.makedirs(target, 0755) > > if not os.path.exists(self.mountmap[x]): > if not self.mountmap[x] == "tmpfs": > - os.makedirs(self.mountmap[x],0755) > + os.makedirs(self.mountmap[x], 0755) > > src=self.mountmap[x] > #print "bind(); src =", src > @@ -910,20 +935,22 @@ class generic_stage_target(generic_target): > self.snapshot_lock_object.read_lock() > if os.uname()[0] == "FreeBSD": > if src == "/dev": > - retval = os.system("mount -t devfs > none " + > - > self.settings["chroot_path"] + src) > + cmd = "mount -t devfs none " + > target > + retval=os.system(cmd) > "because the code use to do it" aside, is there some compelling reason to not use subprocess.Popen here? os.system is probably the worst syscall you can use. It spawns a shell, it will select things like './mount' over '/usr/bin/mount' because you failed to specify the full path to the binary, its not clear what environment the shell is using. Even something like: import subprocess retval = subprocess.call(cmd.split()) would likely avoid these problems unless you are relying on shell expansion in cmd? > else: > - retval = os.system("mount_nullfs " > + src + " " + > - > self.settings["chroot_path"] + src) > + cmd = "mount_nullfs " + src + " " > + target > + retval=os.system(cmd) > else: > if src == "tmpfs": > if "var_tmpfs_portage" in > self.settings: > - retval=os.system("mount -t > tmpfs -o size="+\ > - > self.settings["var_tmpfs_portage"]+"G "+src+" "+\ > - > self.settings["chroot_path"]+x) > + cmd = "mount -t tmpfs -o > size=" + \ > + > self.settings["var_tmpfs_portage"] + "G " + \ > + src + " " + target > + retval=os.system(cmd) > else: > - retval = os.system("mount --bind " > + src + " " + > - > self.settings["chroot_path"] + src) > + cmd = "mount --bind " + src + " " > + target > + #print "bind(); cmd =", cmd > + retval=os.system(cmd) > if retval!=0: > self.unbind() > raise CatalystError,"Couldn't bind mount " > + src > @@ -935,26 +962,25 @@ class generic_stage_target(generic_target): > myrevmounts.reverse() > """ Unmount in reverse order for nested bind-mounts """ > for x in myrevmounts: > - if not os.path.exists(mypath + self.mountmap[x]): > + target = normpath(mypath + self.target_mounts[x]) > + if not os.path.exists(target): > continue > > - if not ismount(mypath + self.mountmap[x]): > + if not ismount(target): > continue > > - retval=os.system("umount "+\ > - os.path.join(mypath, > self.mountmap[x].lstrip(os.path.sep))) > + retval=os.system("umount " + target) > > if retval!=0: > - warn("First attempt to unmount: " + mypath > + > - self.mountmap[x] +" failed.") > + warn("First attempt to unmount: " + target > + " failed.") > warn("Killing any pids still running in > the chroot") > > self.kill_chroot_pids() > > - retval2 = os.system("umount " + mypath + > self.mountmap[x]) > + retval2 = os.system("umount " + target) > if retval2!=0: > ouch=1 > - warn("Couldn't umount bind mount: > " + mypath + self.mountmap[x]) > + warn("Couldn't umount bind mount: > " + target) > > if "SNAPCACHE" in self.settings and x == > "/usr/portage": > try: > diff --git a/modules/stage1_target.py b/modules/stage1_target.py > index aa43926..1e0d874 100644 > --- a/modules/stage1_target.py > +++ b/modules/stage1_target.py > @@ -88,8 +88,9 @@ class stage1_target(generic_stage_target): > os.makedirs(self.settings["stage_path"]+"/proc") > > # alter the mount mappings to bind mount proc onto it > - self.mounts.append("/tmp/stage1root/proc") > - self.mountmap["/tmp/stage1root/proc"]="/proc" > + self.mounts.append("stage1root/proc") > + self.target_mounts["stage1root/proc"] = > "/tmp/stage1root/proc" > + self.mountmap["stage1root/proc"]="/proc" > > def register(foo): > foo.update({"stage1":stage1_target}) > -- > 1.8.3.2 > > >