On Sat, 2013-12-28 at 21:26 -0800, Alec Warner wrote: > On Sat, Dec 28, 2013 at 5:57 PM, Brian Dolbec > > > 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. > > Actually string + string is faster than "%s%s" %(string1, string2) for simple string adition like this example. However %s substitution performs better for larger multiples which occur many, many times throughout the code. So, yes we should clean this up after the current rewrite code is merged in one form or another. > > > > 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? > > > Yes, I have changed the primary chroot run from the old cloned portage spawn()'s to using subprocess.Popen. Coming to you in a future commit waiting it's turn here. I have not yet tackled these os.system() calls. These changes just moved the string addition out of the function call as a preparation for it's replacement. There is no shortage of things that need fixing... There are lots more uses of os.system() below... > > > 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 > > > >