On Tue, Dec 17, 2013 at 05:07:27PM -0800, Brian Dolbec wrote:
> +target_mounts = {
> +	"proc": "/proc",
> + …
> +	"port_logdir": "/var/log/portage",
> +	}
> …
> @@ -173,11 +188,12 @@ class generic_stage_target(generic_target):
>  			file_locate(self.settings,["portage_confdir"],expand=0)
>  
>  		""" Setup our mount points """
> +		self.target_mounts = target_mounts.copy()
>  		if "SNAPCACHE" in self.settings:
>  			self.mounts=["proc", "dev", "portdir", "distdir", "port_tmpdir"]
>  			self.mountmap={"proc":"/proc", "dev":"/dev", "devpts":"/dev/pts",
> -				"portdir":self.settings["snapshot_cache_path"]+"/portage",\
> -				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}
> +				"portdir":normpath(self.settings["snapshot_cache_path"]+"/" + self.settings["repo_name"]),
> + 				"distdir":self.settings["distdir"],"port_tmpdir":"tmpfs"}

Can we do this for the mountmap defaults too, and just override the
mountmap special cases in the SNAPCACHE branch?  I think this would be
a good commit for that (or we can do it in a commit after this).

I don't think the 'portage' → self.settings["repo_name"] replacement
should go in this commit.  It also looks like there is some leading
non-tab whitespace at the beginning of your new distdir line, which
was otherwise unchanged.

> -			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.settings["port_logdir"]

I don't know where we stand on 'x=y' vs 'x = y', but I'd prefer the
latter here.  I also think that the PORT_LOGDIR environment variable
should be:

  self.env["PORT_LOGDIR"] = self.target_mounts["port_logdir"]

Nothing in the chroot should care where the source comes from ;).  I'd
considering renaming mountmap → source_mounts for clarity, using
existing settings to override source_mounts at initialization, and
using source_mounts thereafter.

>  	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]

Might as well shift this src declaration up and use it instead of
self.mountmap[x] earlier.  Not exactly required for this commit, but
you are defining target, so it's not too much of a stretch ;).
 
> -			if "SNAPCACHE" in self.settings and x == "/usr/portage":
> +			#print "bind(); src =", src
> +			if "SNAPCACHE" in self.settings and x == self.settings["portdir"]:

Oops.  x is the mountmap key, so I think this should be 'portdir'.

> +					#print "bind(); cmd =", cmd

If it's not useful enough to print, I don't think we should commit it
;).  I'll be happy once we're using subprocess :p.

A few typos, but I think this makes mount handling much more sane :).

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy