Re: [gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary

[Alec Warner]

On Sat, Dec 28, 2013 at 5:57 PM, Brian Dolbec <dolsen@g.o> 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
>
>
>