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

[Brian Dolbec <dolsen@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 7657 bytes --]

On Sat, 2013-12-28 at 21:26 -0800, Alec Warner wrote:
> On Sat, Dec 28, 2013 at 5:57 PM, Brian Dolbec <dolsen@gentoo.org> 
> 
> 
> 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
>         
>         
> 
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 620 bytes --]