From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id 07A55138247 for ; Sun, 29 Dec 2013 06:01:30 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id 096E2E0B14; Sun, 29 Dec 2013 06:01:27 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 61695E0B14 for ; Sun, 29 Dec 2013 06:01:26 +0000 (UTC) Received: from [192.168.1.210] (S010600222de111ff.vc.shawcable.net [96.49.5.156]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: dolsen) by smtp.gentoo.org (Postfix) with ESMTPSA id 36ED733F754 for ; Sun, 29 Dec 2013 06:01:25 +0000 (UTC) Message-ID: <1388296862.24088.47.camel@big_daddy.dol-sen.ca> Subject: Re: [gentoo-catalyst] [PATCH 5/6] modules/generic_stage_target.py, modules/stage1_target.py: Add a target_mounts dictionary From: Brian Dolbec To: gentoo-catalyst@lists.gentoo.org Date: Sat, 28 Dec 2013 22:01:02 -0800 In-Reply-To: References: <1388282230-3563-1-git-send-email-dolsen@gentoo.org> <1388282230-3563-6-git-send-email-dolsen@gentoo.org> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-Ajf10oQLadh3kGfhyoKa" X-Mailer: Evolution 3.6.4 Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-catalyst@lists.gentoo.org Reply-to: gentoo-catalyst@lists.gentoo.org Mime-Version: 1.0 X-Archives-Salt: 6573ef3e-aa6f-4fbb-8674-b4462c0a640a X-Archives-Hash: 71ef5d708b961859d108abb6310dd1bc --=-Ajf10oQLadh3kGfhyoKa Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sat, 2013-12-28 at 21:26 -0800, Alec Warner wrote: > On Sat, Dec 28, 2013 at 5:57 PM, Brian Dolbec =20 >=20 >=20 > 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') >=20 >=20 > You can use >=20 >=20 > "%s:%s" % (string1, string2) or ":".join(string1, string2) >=20 >=20 > Either are fine, and are nominally better than string + string + > string. >=20 >=20 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. > =20 > =20 > =20 > self.snapshot_lock_object.read_lock() > if os.uname()[0] =3D=3D "FreeBSD": > if src =3D=3D "/dev": > - retval =3D > os.system("mount -t devfs none " + > - > self.settings["chroot_path"] + src) > + cmd =3D "mount -t devfs > none " + target > + retval=3Dos.system(cmd) >=20 >=20 > "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. >=20 >=20 > Even something like: >=20 >=20 > import subprocess > retval =3D subprocess.call(cmd.split()) >=20 >=20 > would likely avoid these problems unless you are relying on shell > expansion in cmd? >=20 >=20 >=20 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... >=20 > =20 > else: > - retval =3D > os.system("mount_nullfs " + src + " " + > - > self.settings["chroot_path"] + src) > + cmd =3D "mount_nullfs " > + src + " " + target > + retval=3Dos.system(cmd) > else: > if src =3D=3D "tmpfs": > if "var_tmpfs_portage" > in self.settings: > - > retval=3Dos.system("mount -t tmpfs -o size=3D"+\ > - > self.settings["var_tmpfs_portage"]+"G "+src+" "+\ > - > self.settings["chroot_path"]+x) > + cmd =3D "mount > -t tmpfs -o size=3D" + \ > + > self.settings["var_tmpfs_portage"] + "G " + \ > + src + > " " + target > + > retval=3Dos.system(cmd) > else: > - retval =3D > os.system("mount --bind " + src + " " + > - > self.settings["chroot_path"] + src) > + cmd =3D "mount --bind " > + src + " " + target > + #print "bind(); cmd > =3D", cmd > + retval=3Dos.system(cmd) > if retval!=3D0: > 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 =3D normpath(mypath + > self.target_mounts[x]) > + if not os.path.exists(target): > continue > =20 > - if not ismount(mypath + > self.mountmap[x]): > + if not ismount(target): > continue > =20 > - retval=3Dos.system("umount "+\ > - os.path.join(mypath, > self.mountmap[x].lstrip(os.path.sep))) > + retval=3Dos.system("umount " + target) > =20 > if retval!=3D0: > - 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") > =20 > self.kill_chroot_pids() > =20 > - retval2 =3D os.system("umount " > + mypath + self.mountmap[x]) > + retval2 =3D os.system("umount " > + target) > if retval2!=3D0: > ouch=3D1 > - warn("Couldn't umount > bind mount: " + mypath + self.mountmap[x]) > + warn("Couldn't umount > bind mount: " + target) > =20 > if "SNAPCACHE" in self.settings and x > =3D=3D "/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): > =20 > os.makedirs(self.settings["stage_path"]+"/proc") > =20 > # alter the mount mappings to bind mount proc > onto it > - self.mounts.append("/tmp/stage1root/proc") > - self.mountmap["/tmp/stage1root/proc"]=3D"/proc" > + self.mounts.append("stage1root/proc") > + self.target_mounts["stage1root/proc"] =3D > "/tmp/stage1root/proc" > + self.mountmap["stage1root/proc"]=3D"/proc" > =20 > def register(foo): > foo.update({"stage1":stage1_target}) > -- > 1.8.3.2 > =20 > =20 >=20 >=20 --=-Ajf10oQLadh3kGfhyoKa Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQF8BAABCgBmBQJSv7qeXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXQ4Njg4RkQxQ0M3MUMxQzA0RUFFQTQyMzcy MjE0RDkwQTAxNEYxN0NCAAoJECIU2QoBTxfLZBwH/igvLGkNbItuNi2Bqa1hvP6t CyraRk49VFKv/7qcwzx9Aga/Qt2TreTGVAgpV6k1i+alqxlZzze1svyIIyMUVXUP gJ6owKRUi5JMj8bZ45XsGxXRPYgepmgaLhdajXojZS2D4NCjztdaYeQdC1JosVch fkMe/GFkW40bxPDJP3exwocPR8xYrLYWpFcM3+f08CrzXpIyHn3aJpckiSqiJ+Ip zVrMfRDMpe2mUXh+PJZp5fl+y0EBLReHytIrYFPvBqO9nwW8aDKLKJm/sKkXNpLT t/HvSLWT5416C4gUSIzFN1HVRyJNIPSmK7NH5TiBscqZGFUzny8VQNO9bS8BvGU= =SO2A -----END PGP SIGNATURE----- --=-Ajf10oQLadh3kGfhyoKa--