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