public inbox for gentoo-catalyst@lists.gentoo.org
 help / color / mirror / Atom feed
* [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare()
@ 2020-10-29 16:16 Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 02/12] catalyst: Rewrite ismount() to use libmount Matt Turner
                   ` (10 more replies)
  0 siblings, 11 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

Modern Python allows us to do this in a much cleaner way.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/support.py | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/catalyst/support.py b/catalyst/support.py
index f49315a5..4458ed20 100644
--- a/catalyst/support.py
+++ b/catalyst/support.py
@@ -5,6 +5,7 @@ import os
 import re
 import shutil
 import time
+from pathlib import Path
 from subprocess import Popen
 
 from catalyst import log
@@ -179,31 +180,20 @@ def read_makeconf(mymakeconffile):
         return makeconf
 
 
-def pathcompare(path1, path2):
-    # Change double slashes to slash
-    path1 = re.sub(r"//", r"/", path1)
-    path2 = re.sub(r"//", r"/", path2)
-    # Removing ending slash
-    path1 = re.sub("/$", "", path1)
-    path2 = re.sub("/$", "", path2)
-
-    if path1 == path2:
-        return 1
-    return 0
-
-
 def ismount(path):
     """Like os.path.ismount, but also support bind mounts"""
     if os.path.ismount(path):
-        return 1
+        return True
+
     a = os.popen("mount")
     mylines = a.readlines()
     a.close()
     for line in mylines:
         mysplit = line.split()
-        if pathcompare(path, mysplit[2]):
-            return 1
-    return 0
+        if Path(path) == Path(mysplit[2]):
+            return True
+
+    return False
 
 
 def addl_arg_parse(myspec, addlargs, requiredspec, validspec):
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 02/12] catalyst: Rewrite ismount() to use libmount
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 03/12] catalyst: Use libmount for handling mounts Matt Turner
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

libmount is provided by util-linux, so this adds a dependency on
sys-apps/util-linux[python]. A later patch will make more extensive use
of this API.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 README              |  2 +-
 catalyst/support.py | 14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/README b/README
index 1cceb63e..594de9e1 100644
--- a/README
+++ b/README
@@ -17,7 +17,7 @@ simple and reproducable manner. Use at your own risk.
 Requirements
 =======================
 
-- Python 3.6 or greater
+- Python 3.8 or greater
 - A generic stage3 tarball for your architecture
 - A squashfs ebuild repository snapshot
   - Or an ebuild git repo with sys-fs/squashfs-tools-ng and dev-vcs/git
diff --git a/catalyst/support.py b/catalyst/support.py
index 4458ed20..ddbd9ab9 100644
--- a/catalyst/support.py
+++ b/catalyst/support.py
@@ -8,6 +8,8 @@ import time
 from pathlib import Path
 from subprocess import Popen
 
+import libmount
+
 from catalyst import log
 
 BASH_BINARY = "/bin/bash"
@@ -182,15 +184,13 @@ def read_makeconf(mymakeconffile):
 
 def ismount(path):
     """Like os.path.ismount, but also support bind mounts"""
-    if os.path.ismount(path):
+    path = Path(path)
+    if path.is_mount():
         return True
 
-    a = os.popen("mount")
-    mylines = a.readlines()
-    a.close()
-    for line in mylines:
-        mysplit = line.split()
-        if Path(path) == Path(mysplit[2]):
+    cxt = libmount.Context()
+    while (fs := cxt.mtab.next_fs()) is not None:
+        if path == Path(fs.target):
             return True
 
     return False
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 03/12] catalyst: Use libmount for handling mounts
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 02/12] catalyst: Rewrite ismount() to use libmount Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 04/12] catalyst: Move action_sequence out of self.settings[] Matt Turner
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

Handle the mounts/unmounts in all in process rather than shelling out
(pun intended!) to an external program.

While we're here, change some log.notice to log.debug since those cases
are normal and expected.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/base/stagebase.py | 57 +++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index e71ce344..73eacfbe 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -6,6 +6,7 @@ import sys
 
 from pathlib import Path
 
+import libmount
 import toml
 
 from snakeoil import fileutils
@@ -853,7 +854,8 @@ class StageBase(TargetBase, ClearBase, GenBase):
 
             source = str(self.mount[x]['source'])
             target = self.settings['chroot_path'] + str(self.mount[x]['target'])
-            mount = ['mount']
+            fstype = ''
+            options = ''
 
             log.debug('bind %s: "%s" -> "%s"', x, source, target)
 
@@ -861,18 +863,20 @@ class StageBase(TargetBase, ClearBase, GenBase):
                 if 'var_tmpfs_portage' not in self.settings:
                     continue
 
-                mount += ['-t', 'tmpfs', '-o',
-                          f"size={self.settings['var_tmpfs_portage']}G"]
+                fstype = 'tmpfs'
+                options = f"size={self.settings['var_tmpfs_portage']}G"
             elif source == 'tmpfs':
-                mount += ['-t', 'tmpfs']
+                fstype = 'tmpfs'
             elif source == 'shm':
-                mount += ['-t', 'tmpfs', '-o', 'noexec,nosuid,nodev']
+                fstype = 'tmpfs'
+                options = 'noexec,nosuid,nodev'
             else:
                 source_path = Path(self.mount[x]['source'])
                 if source_path.suffix == '.sqfs':
-                    mount += ['-o', 'ro']
+                    fstype = 'squashfs'
+                    options = 'ro,loop'
                 else:
-                    mount.append('--bind')
+                    options = 'bind'
 
                     # We may need to create the source of the bind mount. E.g., in the
                     # case of an empty package cache we must create the directory that
@@ -881,38 +885,47 @@ class StageBase(TargetBase, ClearBase, GenBase):
 
             Path(target).mkdir(mode=0o755, parents=True, exist_ok=True)
 
-            cmd(mount + [source, target], env=self.env, fail_func=self.unbind)
+            try:
+                cxt = libmount.Context(source=source, target=target,
+                                       fstype=fstype, options=options)
+                cxt.mount()
+            except OSError as e:
+                self.unbind()
+                raise CatalystError(f"Couldn't mount: {source}, {e.strerror}")
 
     def unbind(self):
-        ouch = 0
-        mypath = self.settings["chroot_path"]
+        chroot_path = self.settings["chroot_path"]
+        umount_failed = False
 
         # Unmount in reverse order
-        for x in [x for x in reversed(self.mount) if self.mount[x]['enable']]:
-            target = normpath(mypath + self.mount[x]['target'])
-            if not os.path.exists(target):
-                log.notice('%s does not exist. Skipping', target)
+        for target in [Path(chroot_path + self.mount[x]['target'])
+                       for x in reversed(self.mount)
+                       if self.mount[x]['enable']]:
+            if not target.exists():
+                log.debug('%s does not exist. Skipping', target)
                 continue
 
             if not ismount(target):
-                log.notice('%s is not a mount point. Skipping', target)
+                log.debug('%s is not a mount point. Skipping', target)
                 continue
 
             try:
-                cmd(['umount', target], env=self.env)
-            except CatalystError:
+                cxt = libmount.Context(target=str(target))
+                cxt.umount()
+            except OSError:
                 log.warning('First attempt to unmount failed: %s', target)
                 log.warning('Killing any pids still running in the chroot')
 
                 self.kill_chroot_pids()
 
                 try:
-                    cmd(['umount', target], env=self.env)
-                except CatalystError:
-                    ouch = 1
-                    log.warning("Couldn't umount bind mount: %s", target)
+                    cxt.umount()
+                except OSError as e:
+                    umount_failed = True
+                    log.warning("Couldn't umount: %s, %s", target,
+                                e.strerror)
 
-        if ouch:
+        if umount_failed:
             # if any bind mounts really failed, then we need to raise
             # this to potentially prevent an upcoming bash stage cleanup script
             # from wiping our bind mounts.
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 04/12] catalyst: Move action_sequence out of self.settings[]
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 02/12] catalyst: Rewrite ismount() to use libmount Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 03/12] catalyst: Use libmount for handling mounts Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 05/12] catalyst: Use .extend() and .append() for action_sequence Matt Turner
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

This self.settings[] dictionary is very similar to the god object
anti-pattern. Moving action_sequence out it starts us down the road of
fixing this.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/base/stagebase.py        | 17 +++++++++--------
 catalyst/targets/embedded.py      |  2 +-
 catalyst/targets/livecd_stage1.py |  2 +-
 catalyst/targets/livecd_stage2.py |  6 +++---
 catalyst/targets/netboot.py       |  2 +-
 catalyst/targets/stage1.py        | 12 ++++++------
 catalyst/targets/stage4.py        |  2 +-
 7 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index 73eacfbe..801df2fb 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -64,6 +64,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
             "portage_overlay",
             "portage_prefix",
         ])
+        self.action_sequence = []
 
         self.set_valid_build_kernel_vars(addlargs)
         TargetBase.__init__(self, myspec, addlargs)
@@ -477,13 +478,13 @@ class StageBase(TargetBase, ClearBase, GenBase):
         Or it calls the normal set_action_sequence() for the target stage.
         """
         if "purgeonly" in self.settings["options"]:
-            self.settings["action_sequence"] = ["remove_chroot"]
+            self.action_sequence = ["remove_chroot"]
             return
         self.set_action_sequence()
 
     def set_action_sequence(self):
         """Set basic stage1, 2, 3 action sequences"""
-        self.settings['action_sequence'] = [
+        self.action_sequence = [
             "unpack",
             "setup_confdir",
             "portage_overlay",
@@ -499,14 +500,14 @@ class StageBase(TargetBase, ClearBase, GenBase):
 
     def set_completion_action_sequences(self):
         if "fetch" not in self.settings["options"]:
-            self.settings["action_sequence"].append("capture")
+            self.action_sequence.append("capture")
         if "keepwork" in self.settings["options"]:
-            self.settings["action_sequence"].append("clear_autoresume")
+            self.action_sequence.append("clear_autoresume")
         elif "seedcache" in self.settings["options"]:
-            self.settings["action_sequence"].append("remove_autoresume")
+            self.action_sequence.append("remove_autoresume")
         else:
-            self.settings["action_sequence"].append("remove_autoresume")
-            self.settings["action_sequence"].append("remove_chroot")
+            self.action_sequence.append("remove_autoresume")
+            self.action_sequence.append("remove_chroot")
 
     def set_use(self):
         use = self.settings["spec_prefix"] + "/use"
@@ -1380,7 +1381,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
             self.purge()
 
         failure = False
-        for x in self.settings["action_sequence"]:
+        for x in self.action_sequence:
             log.notice('--- Running action sequence: %s', x)
             sys.stdout.flush()
             try:
diff --git a/catalyst/targets/embedded.py b/catalyst/targets/embedded.py
index 99739512..3899cf1b 100644
--- a/catalyst/targets/embedded.py
+++ b/catalyst/targets/embedded.py
@@ -41,7 +41,7 @@ class embedded(StageBase):
         StageBase.__init__(self, spec, addlargs)
 
     def set_action_sequence(self):
-        self.settings['action_sequence'] = [
+        self.action_sequence = [
             "unpack",
             "config_profile_link",
             "setup_confdir",
diff --git a/catalyst/targets/livecd_stage1.py b/catalyst/targets/livecd_stage1.py
index f0b6be8b..b8c26cb1 100644
--- a/catalyst/targets/livecd_stage1.py
+++ b/catalyst/targets/livecd_stage1.py
@@ -23,7 +23,7 @@ class livecd_stage1(StageBase):
         StageBase.__init__(self, spec, addlargs)
 
     def set_action_sequence(self):
-        self.settings['action_sequence'] = [
+        self.action_sequence = [
             "unpack",
             "config_profile_link",
             "setup_confdir",
diff --git a/catalyst/targets/livecd_stage2.py b/catalyst/targets/livecd_stage2.py
index 88c0d95c..cac16b6e 100644
--- a/catalyst/targets/livecd_stage2.py
+++ b/catalyst/targets/livecd_stage2.py
@@ -87,7 +87,7 @@ class livecd_stage2(StageBase):
                                     print_traceback=True)
 
     def set_action_sequence(self):
-        self.settings['action_sequence'] = [
+        self.action_sequence = [
             "unpack",
             "config_profile_link",
             "setup_confdir",
@@ -99,7 +99,7 @@ class livecd_stage2(StageBase):
             "build_kernel"
         ]
         if "fetch" not in self.settings["options"]:
-            self.settings['action_sequence'] += [
+            self.action_sequence += [
                 "bootloader",
                 "preclean",
                 "livecd_update",
@@ -115,4 +115,4 @@ class livecd_stage2(StageBase):
                 "setup_overlay",
                 "create_iso",
             ]
-        self.settings["action_sequence"].append("clear_autoresume")
+        self.action_sequence.append("clear_autoresume")
diff --git a/catalyst/targets/netboot.py b/catalyst/targets/netboot.py
index 5620e0d3..61583f0d 100644
--- a/catalyst/targets/netboot.py
+++ b/catalyst/targets/netboot.py
@@ -160,7 +160,7 @@ class netboot(StageBase):
         self.resume.enable("empty")
 
     def set_action_sequence(self):
-        self.settings['action_sequence'] = [
+        self.action_sequence = [
             "unpack",
             "config_profile_link",
             "setup_confdir",
diff --git a/catalyst/targets/stage1.py b/catalyst/targets/stage1.py
index 2c09a41f..89b30fe1 100644
--- a/catalyst/targets/stage1.py
+++ b/catalyst/targets/stage1.py
@@ -87,15 +87,15 @@ class stage1(StageBase):
         chroot for re-use in stage2 without the need to unpack it.
         '''
         if "fetch" not in self.settings["options"]:
-            self.settings["action_sequence"].append("capture")
+            self.action_sequence.append("capture")
         if "keepwork" in self.settings["options"]:
-            self.settings["action_sequence"].append("clear_autoresume")
+            self.action_sequence.append("clear_autoresume")
         elif "seedcache" in self.settings["options"]:
-            self.settings["action_sequence"].append("remove_autoresume")
-            self.settings["action_sequence"].append("clean_stage1")
+            self.action_sequence.append("remove_autoresume")
+            self.action_sequence.append("clean_stage1")
         else:
-            self.settings["action_sequence"].append("remove_autoresume")
-            self.settings["action_sequence"].append("remove_chroot")
+            self.action_sequence.append("remove_autoresume")
+            self.action_sequence.append("remove_chroot")
 
     def clean_stage1(self):
         '''seedcache is enabled, so salvage the /tmp/stage1root,
diff --git a/catalyst/targets/stage4.py b/catalyst/targets/stage4.py
index 346c0845..eef24a73 100644
--- a/catalyst/targets/stage4.py
+++ b/catalyst/targets/stage4.py
@@ -39,7 +39,7 @@ class stage4(StageBase):
         self.settings["cleanables"].remove('/etc/resolv.conf')
 
     def set_action_sequence(self):
-        self.settings['action_sequence'] = [
+        self.action_sequence = [
             "unpack",
             "config_profile_link",
             "setup_confdir",
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 05/12] catalyst: Use .extend() and .append() for action_sequence
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
                   ` (2 preceding siblings ...)
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 04/12] catalyst: Move action_sequence out of self.settings[] Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 06/12] catalyst: Split action_sequence into prepare/build/finish Matt Turner
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

Ensures that we don't overwrite and lose some settings.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/base/stagebase.py        | 6 +++---
 catalyst/targets/embedded.py      | 4 ++--
 catalyst/targets/livecd_stage1.py | 4 ++--
 catalyst/targets/livecd_stage2.py | 8 ++++----
 catalyst/targets/netboot.py       | 4 ++--
 catalyst/targets/stage4.py        | 4 ++--
 6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index 801df2fb..46b7c59c 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -478,13 +478,13 @@ class StageBase(TargetBase, ClearBase, GenBase):
         Or it calls the normal set_action_sequence() for the target stage.
         """
         if "purgeonly" in self.settings["options"]:
-            self.action_sequence = ["remove_chroot"]
+            self.action_sequence.append("remove_chroot")
             return
         self.set_action_sequence()
 
     def set_action_sequence(self):
         """Set basic stage1, 2, 3 action sequences"""
-        self.action_sequence = [
+        self.action_sequence.extend([
             "unpack",
             "setup_confdir",
             "portage_overlay",
@@ -495,7 +495,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
             "preclean",
             "unbind",
             "clean",
-        ]
+        ])
         self.set_completion_action_sequences()
 
     def set_completion_action_sequences(self):
diff --git a/catalyst/targets/embedded.py b/catalyst/targets/embedded.py
index 3899cf1b..75eb68e4 100644
--- a/catalyst/targets/embedded.py
+++ b/catalyst/targets/embedded.py
@@ -41,7 +41,7 @@ class embedded(StageBase):
         StageBase.__init__(self, spec, addlargs)
 
     def set_action_sequence(self):
-        self.action_sequence = [
+        self.action_sequence.extend([
             "unpack",
             "config_profile_link",
             "setup_confdir",
@@ -60,7 +60,7 @@ class embedded(StageBase):
             "clean",
             "capture",
             "clear_autoresume",
-        ]
+        ])
 
     def set_stage_path(self):
         self.settings["stage_path"] = normpath(
diff --git a/catalyst/targets/livecd_stage1.py b/catalyst/targets/livecd_stage1.py
index b8c26cb1..9dbfa506 100644
--- a/catalyst/targets/livecd_stage1.py
+++ b/catalyst/targets/livecd_stage1.py
@@ -23,7 +23,7 @@ class livecd_stage1(StageBase):
         StageBase.__init__(self, spec, addlargs)
 
     def set_action_sequence(self):
-        self.action_sequence = [
+        self.action_sequence.extend([
             "unpack",
             "config_profile_link",
             "setup_confdir",
@@ -34,7 +34,7 @@ class livecd_stage1(StageBase):
             "build_packages",
             "unbind",
             "clean",
-        ]
+        ])
         self.set_completion_action_sequences()
 
     def set_spec_prefix(self):
diff --git a/catalyst/targets/livecd_stage2.py b/catalyst/targets/livecd_stage2.py
index cac16b6e..c9b5ce08 100644
--- a/catalyst/targets/livecd_stage2.py
+++ b/catalyst/targets/livecd_stage2.py
@@ -87,7 +87,7 @@ class livecd_stage2(StageBase):
                                     print_traceback=True)
 
     def set_action_sequence(self):
-        self.action_sequence = [
+        self.action_sequence.extend([
             "unpack",
             "config_profile_link",
             "setup_confdir",
@@ -97,9 +97,9 @@ class livecd_stage2(StageBase):
             "setup_environment",
             "run_local",
             "build_kernel"
-        ]
+        ])
         if "fetch" not in self.settings["options"]:
-            self.action_sequence += [
+            self.action_sequence.extend([
                 "bootloader",
                 "preclean",
                 "livecd_update",
@@ -114,5 +114,5 @@ class livecd_stage2(StageBase):
                 "target_setup",
                 "setup_overlay",
                 "create_iso",
-            ]
+            ])
         self.action_sequence.append("clear_autoresume")
diff --git a/catalyst/targets/netboot.py b/catalyst/targets/netboot.py
index 61583f0d..e5c6d43c 100644
--- a/catalyst/targets/netboot.py
+++ b/catalyst/targets/netboot.py
@@ -160,7 +160,7 @@ class netboot(StageBase):
         self.resume.enable("empty")
 
     def set_action_sequence(self):
-        self.action_sequence = [
+        self.action_sequence.extend([
             "unpack",
             "config_profile_link",
             "setup_confdir",
@@ -179,4 +179,4 @@ class netboot(StageBase):
             "unbind",
             "clean",
             "clear_autoresume",
-        ]
+        ])
diff --git a/catalyst/targets/stage4.py b/catalyst/targets/stage4.py
index eef24a73..bb20be79 100644
--- a/catalyst/targets/stage4.py
+++ b/catalyst/targets/stage4.py
@@ -39,7 +39,7 @@ class stage4(StageBase):
         self.settings["cleanables"].remove('/etc/resolv.conf')
 
     def set_action_sequence(self):
-        self.action_sequence = [
+        self.action_sequence.extend([
             "unpack",
             "config_profile_link",
             "setup_confdir",
@@ -59,5 +59,5 @@ class stage4(StageBase):
             "remove",
             "empty",
             "clean",
-        ]
+        ])
         self.set_completion_action_sequences()
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 06/12] catalyst: Split action_sequence into prepare/build/finish
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
                   ` (3 preceding siblings ...)
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 05/12] catalyst: Use .extend() and .append() for action_sequence Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 07/12] catalyst: Factor out run_sequence() Matt Turner
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

We want to run the "build" sequence in a different mount namespace from
the "prepare" and "finish" sequences, so this splits action_sequence
into those groupings.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/base/stagebase.py        | 24 +++++++++++++++---------
 catalyst/targets/embedded.py      |  6 +++++-
 catalyst/targets/livecd_stage1.py |  6 +++++-
 catalyst/targets/livecd_stage2.py | 10 +++++++---
 catalyst/targets/netboot.py       |  6 +++++-
 catalyst/targets/stage1.py        | 12 ++++++------
 catalyst/targets/stage4.py        |  6 +++++-
 7 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index 46b7c59c..75c84baa 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -64,7 +64,9 @@ class StageBase(TargetBase, ClearBase, GenBase):
             "portage_overlay",
             "portage_prefix",
         ])
-        self.action_sequence = []
+        self.prepare_sequence = []
+        self.build_sequence = []
+        self.finish_sequence = []
 
         self.set_valid_build_kernel_vars(addlargs)
         TargetBase.__init__(self, myspec, addlargs)
@@ -478,36 +480,40 @@ class StageBase(TargetBase, ClearBase, GenBase):
         Or it calls the normal set_action_sequence() for the target stage.
         """
         if "purgeonly" in self.settings["options"]:
-            self.action_sequence.append("remove_chroot")
+            self.build_sequence.append("remove_chroot")
             return
         self.set_action_sequence()
 
     def set_action_sequence(self):
         """Set basic stage1, 2, 3 action sequences"""
-        self.action_sequence.extend([
+        self.prepare_sequence.extend([
             "unpack",
             "setup_confdir",
             "portage_overlay",
+        ])
+        self.build_sequence.extend([
             "bind",
             "chroot_setup",
             "setup_environment",
             "run_local",
             "preclean",
             "unbind",
+        ])
+        self.finish_sequence.extend([
             "clean",
         ])
         self.set_completion_action_sequences()
 
     def set_completion_action_sequences(self):
         if "fetch" not in self.settings["options"]:
-            self.action_sequence.append("capture")
+            self.finish_sequence.append("capture")
         if "keepwork" in self.settings["options"]:
-            self.action_sequence.append("clear_autoresume")
+            self.finish_sequence.append("clear_autoresume")
         elif "seedcache" in self.settings["options"]:
-            self.action_sequence.append("remove_autoresume")
+            self.finish_sequence.append("remove_autoresume")
         else:
-            self.action_sequence.append("remove_autoresume")
-            self.action_sequence.append("remove_chroot")
+            self.finish_sequence.append("remove_autoresume")
+            self.finish_sequence.append("remove_chroot")
 
     def set_use(self):
         use = self.settings["spec_prefix"] + "/use"
@@ -1381,7 +1387,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
             self.purge()
 
         failure = False
-        for x in self.action_sequence:
+        for x in self.prepare_sequence + self.build_sequence + self.finish_sequence:
             log.notice('--- Running action sequence: %s', x)
             sys.stdout.flush()
             try:
diff --git a/catalyst/targets/embedded.py b/catalyst/targets/embedded.py
index 75eb68e4..1314ce7c 100644
--- a/catalyst/targets/embedded.py
+++ b/catalyst/targets/embedded.py
@@ -41,11 +41,13 @@ class embedded(StageBase):
         StageBase.__init__(self, spec, addlargs)
 
     def set_action_sequence(self):
-        self.action_sequence.extend([
+        self.prepare_sequence.extend([
             "unpack",
             "config_profile_link",
             "setup_confdir",
             "portage_overlay",
+        ])
+        self.build_sequence.extend([
             "bind",
             "chroot_setup",
             "setup_environment",
@@ -55,6 +57,8 @@ class embedded(StageBase):
             "fsscript",
             "unmerge",
             "unbind",
+        ])
+        self.finish_sequence.extend([
             "remove",
             "empty",
             "clean",
diff --git a/catalyst/targets/livecd_stage1.py b/catalyst/targets/livecd_stage1.py
index 9dbfa506..81367053 100644
--- a/catalyst/targets/livecd_stage1.py
+++ b/catalyst/targets/livecd_stage1.py
@@ -23,16 +23,20 @@ class livecd_stage1(StageBase):
         StageBase.__init__(self, spec, addlargs)
 
     def set_action_sequence(self):
-        self.action_sequence.extend([
+        self.prepare_sequence.extend([
             "unpack",
             "config_profile_link",
             "setup_confdir",
             "portage_overlay",
+        ])
+        self.build_sequence.extend([
             "bind",
             "chroot_setup",
             "setup_environment",
             "build_packages",
             "unbind",
+        ])
+        self.finish_sequence.extend([
             "clean",
         ])
         self.set_completion_action_sequences()
diff --git a/catalyst/targets/livecd_stage2.py b/catalyst/targets/livecd_stage2.py
index c9b5ce08..f6c14919 100644
--- a/catalyst/targets/livecd_stage2.py
+++ b/catalyst/targets/livecd_stage2.py
@@ -87,11 +87,13 @@ class livecd_stage2(StageBase):
                                     print_traceback=True)
 
     def set_action_sequence(self):
-        self.action_sequence.extend([
+        self.prepare_sequence.extend([
             "unpack",
             "config_profile_link",
             "setup_confdir",
             "portage_overlay",
+        ])
+        self.build_sequence.extend([
             "bind",
             "chroot_setup",
             "setup_environment",
@@ -99,7 +101,7 @@ class livecd_stage2(StageBase):
             "build_kernel"
         ])
         if "fetch" not in self.settings["options"]:
-            self.action_sequence.extend([
+            self.build_sequence.extend([
                 "bootloader",
                 "preclean",
                 "livecd_update",
@@ -108,6 +110,8 @@ class livecd_stage2(StageBase):
                 "rcupdate",
                 "unmerge",
                 "unbind",
+            ])
+            self.finish_sequence.extend([
                 "remove",
                 "empty",
                 "clean",
@@ -115,4 +119,4 @@ class livecd_stage2(StageBase):
                 "setup_overlay",
                 "create_iso",
             ])
-        self.action_sequence.append("clear_autoresume")
+        self.finish_sequence.append("clear_autoresume")
diff --git a/catalyst/targets/netboot.py b/catalyst/targets/netboot.py
index e5c6d43c..9a0a4156 100644
--- a/catalyst/targets/netboot.py
+++ b/catalyst/targets/netboot.py
@@ -160,11 +160,13 @@ class netboot(StageBase):
         self.resume.enable("empty")
 
     def set_action_sequence(self):
-        self.action_sequence.extend([
+        self.prepare_sequence.extend([
             "unpack",
             "config_profile_link",
             "setup_confdir",
             "portage_overlay",
+        ])
+        self.build_sequence.extend([
             "bind",
             "chroot_setup",
             "setup_environment",
@@ -177,6 +179,8 @@ class netboot(StageBase):
             "remove",
             "empty",
             "unbind",
+        ])
+        self.finish_sequence.extend([
             "clean",
             "clear_autoresume",
         ])
diff --git a/catalyst/targets/stage1.py b/catalyst/targets/stage1.py
index 89b30fe1..be3eae93 100644
--- a/catalyst/targets/stage1.py
+++ b/catalyst/targets/stage1.py
@@ -87,15 +87,15 @@ class stage1(StageBase):
         chroot for re-use in stage2 without the need to unpack it.
         '''
         if "fetch" not in self.settings["options"]:
-            self.action_sequence.append("capture")
+            self.finish_sequence.append("capture")
         if "keepwork" in self.settings["options"]:
-            self.action_sequence.append("clear_autoresume")
+            self.finish_sequence.append("clear_autoresume")
         elif "seedcache" in self.settings["options"]:
-            self.action_sequence.append("remove_autoresume")
-            self.action_sequence.append("clean_stage1")
+            self.finish_sequence.append("remove_autoresume")
+            self.finish_sequence.append("clean_stage1")
         else:
-            self.action_sequence.append("remove_autoresume")
-            self.action_sequence.append("remove_chroot")
+            self.finish_sequence.append("remove_autoresume")
+            self.finish_sequence.append("remove_chroot")
 
     def clean_stage1(self):
         '''seedcache is enabled, so salvage the /tmp/stage1root,
diff --git a/catalyst/targets/stage4.py b/catalyst/targets/stage4.py
index bb20be79..78a5c780 100644
--- a/catalyst/targets/stage4.py
+++ b/catalyst/targets/stage4.py
@@ -39,11 +39,13 @@ class stage4(StageBase):
         self.settings["cleanables"].remove('/etc/resolv.conf')
 
     def set_action_sequence(self):
-        self.action_sequence.extend([
+        self.prepare_sequence.extend([
             "unpack",
             "config_profile_link",
             "setup_confdir",
             "portage_overlay",
+        ])
+        self.build_sequence.extend([
             "bind",
             "chroot_setup",
             "setup_environment",
@@ -56,6 +58,8 @@ class stage4(StageBase):
             "rcupdate",
             "unmerge",
             "unbind",
+        ])
+        self.finish_sequence.extend([
             "remove",
             "empty",
             "clean",
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 07/12] catalyst: Factor out run_sequence()
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
                   ` (4 preceding siblings ...)
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 06/12] catalyst: Split action_sequence into prepare/build/finish Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 08/12] catalyst: Add and use namespace context manager Matt Turner
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

This is preparation for the next patch, which will run the build
sequence in a separate mount namespace.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/base/stagebase.py | 40 ++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index 75c84baa..06ec8727 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -1362,6 +1362,22 @@ class StageBase(TargetBase, ClearBase, GenBase):
 
         log.debug('setup_environment(); env = %r', self.env)
 
+    def run_sequence(self, sequence):
+        for func in sequence:
+            log.notice('--- Running action sequence: %s', func)
+            sys.stdout.flush()
+            try:
+                getattr(self, func)()
+            except LockInUse:
+                log.error('Unable to aquire the lock...')
+                return False
+            except Exception:
+                log.error('Exception running action sequence %s',
+                          func, exc_info=True)
+                return False
+
+        return True
+
     def run(self):
         self.chroot_lock.write_lock()
 
@@ -1386,26 +1402,16 @@ class StageBase(TargetBase, ClearBase, GenBase):
             log.info('StageBase: run() purge')
             self.purge()
 
-        failure = False
-        for x in self.prepare_sequence + self.build_sequence + self.finish_sequence:
-            log.notice('--- Running action sequence: %s', x)
-            sys.stdout.flush()
-            try:
-                getattr(self, x)()
-            except LockInUse:
-                log.error('Unable to aquire the lock...')
-                failure = True
-                break
-            except Exception:
-                log.error('Exception running action sequence %s',
-                          x, exc_info=True)
-                failure = True
-                break
+        if not self.run_sequence(self.prepare_sequence):
+            return False
 
-        if failure:
-            log.notice('Cleaning up... Running unbind()')
+        if not self.run_sequence(self.build_sequence):
             self.unbind()
             return False
+
+        if not self.run_sequence(self.finish_sequence):
+            return False
+
         return True
 
     def unmerge(self):
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 08/12] catalyst: Add and use namespace context manager
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
                   ` (5 preceding siblings ...)
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 07/12] catalyst: Factor out run_sequence() Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 09/12] catalyst: Run the build sequence in new mount namespace Matt Turner
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

Wraps snakeoil's simple_unshare; returns to the previous namespaces on
context exit. Will be used by the next commit.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/context.py | 32 ++++++++++++++++++++++++++++++++
 catalyst/main.py    | 17 +++++++----------
 2 files changed, 39 insertions(+), 10 deletions(-)
 create mode 100644 catalyst/context.py

diff --git a/catalyst/context.py b/catalyst/context.py
new file mode 100644
index 00000000..936b5c6b
--- /dev/null
+++ b/catalyst/context.py
@@ -0,0 +1,32 @@
+
+import contextlib
+import os
+
+from snakeoil.process.namespaces import setns, simple_unshare
+
+@contextlib.contextmanager
+def namespace(mount=False, uts=False, ipc=False, net=False, pid=False,
+              user=False, hostname=None):
+    namespaces = {
+        (mount, "mnt"):  None,
+        (uts,   "uts"):  None,
+        (ipc,   "ipc"):  None,
+        (net,   "net"):  None,
+        (pid,   "pid"):  None,
+        (user,  "user"): None,
+    }
+
+    # Save fds of current namespaces
+    for ns in [ns for ns in namespaces if ns[0]]:
+        fp = open(f"/proc/self/ns/{ns[1]}")
+        namespaces[ns] = fp
+
+    simple_unshare(mount=mount, uts=uts, ipc=ipc, net=net, pid=pid, user=user,
+                   hostname=hostname)
+    try:
+        yield None
+    finally:
+        for ns in [ns for ns in namespaces if ns[0]]:
+            fp = namespaces[ns]
+            setns(fp.fileno(), 0)
+            fp.close()
diff --git a/catalyst/main.py b/catalyst/main.py
index 543895c6..93a4a0d3 100644
--- a/catalyst/main.py
+++ b/catalyst/main.py
@@ -7,14 +7,13 @@ import textwrap
 
 import toml
 
-from snakeoil.process import namespaces
-
 from DeComp.definitions import (COMPRESS_DEFINITIONS, DECOMPRESS_DEFINITIONS,
                                 CONTENTS_DEFINITIONS)
 from DeComp.contents import ContentsMap
 
 from catalyst import log
 import catalyst.config
+from catalyst.context import namespace
 from catalyst.defaults import (confdefaults, option_messages,
                                DEFAULT_CONFIG_FILE, valid_config_file_values)
 from catalyst.support import CatalystError
@@ -356,15 +355,13 @@ def _main(parser, opts):
     # use pid & user namespaces, but snakeoil's namespace module has signal
     # transfer issues (CTRL+C doesn't propagate), and user namespaces need
     # more work due to Gentoo build process (uses sudo/root/portage).
-    namespaces.simple_unshare(
-        mount=True, uts=True, ipc=True, pid=False, net=False, user=False,
-        hostname='catalyst')
+    with namespace(mount=True, uts=True, ipc=True, hostname='catalyst'):
+        # everything is setup, so the build is a go
+        try:
+            success = build_target(addlargs)
+        except KeyboardInterrupt:
+            log.critical('Catalyst build aborted due to user interrupt (Ctrl-C)')
 
-    # everything is setup, so the build is a go
-    try:
-        success = build_target(addlargs)
-    except KeyboardInterrupt:
-        log.critical('Catalyst build aborted due to user interrupt (Ctrl-C)')
     if not success:
         sys.exit(2)
     sys.exit(0)
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 09/12] catalyst: Run the build sequence in new mount namespace
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
                   ` (6 preceding siblings ...)
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 08/12] catalyst: Add and use namespace context manager Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 10/12] catalyst: Remove kill_support_pids() Matt Turner
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

Catalyst has a lot of code to unmount the bind mounts it's made, and
then more to try harder when something fails. This is important because
if bind mounts still exist within the chroot when clean up happens,
files outside of the chroot on the host system can inadvertently be
deleted. E.g., distfiles, binpkgs, kerncache.

Running the build sequence (the steps that need bind mounts) within a
mount namespace and exiting the mount namespace when finished ensures
that clean up can never accidentally delete files outside the chroot.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/base/stagebase.py | 8 +++++---
 catalyst/main.py           | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index 06ec8727..ec9a8f06 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -15,6 +15,7 @@ from snakeoil.osutils import pjoin
 from DeComp.compress import CompressMap
 
 from catalyst import log
+from catalyst.context import namespace
 from catalyst.defaults import (confdefaults, MOUNT_DEFAULTS, PORT_LOGDIR_CLEAN)
 from catalyst.support import (CatalystError, file_locate, normpath,
                               cmd, read_makeconf, ismount, file_check,
@@ -1405,9 +1406,10 @@ class StageBase(TargetBase, ClearBase, GenBase):
         if not self.run_sequence(self.prepare_sequence):
             return False
 
-        if not self.run_sequence(self.build_sequence):
-            self.unbind()
-            return False
+        with namespace(mount=True):
+            if not self.run_sequence(self.build_sequence):
+                self.unbind()
+                return False
 
         if not self.run_sequence(self.finish_sequence):
             return False
diff --git a/catalyst/main.py b/catalyst/main.py
index 93a4a0d3..5536471a 100644
--- a/catalyst/main.py
+++ b/catalyst/main.py
@@ -355,7 +355,7 @@ def _main(parser, opts):
     # use pid & user namespaces, but snakeoil's namespace module has signal
     # transfer issues (CTRL+C doesn't propagate), and user namespaces need
     # more work due to Gentoo build process (uses sudo/root/portage).
-    with namespace(mount=True, uts=True, ipc=True, hostname='catalyst'):
+    with namespace(uts=True, ipc=True, hostname='catalyst'):
         # everything is setup, so the build is a go
         try:
             success = build_target(addlargs)
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 10/12] catalyst: Remove kill_support_pids()
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
                   ` (7 preceding siblings ...)
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 09/12] catalyst: Run the build sequence in new mount namespace Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 11/12] catalyst: Remove mount_safety_check() Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 12/12] catalyst: Drop unbind() Matt Turner
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

mount_namespaces(7) says

	A mount ceases to be a member of a peer group when either the
	mount is explicitly unmounted, or when the mount is implicitly
	unmounted because a mount namespace is removed (because it has
	no more member processes).

Now that the build sequence is executed in its own mount namespace, the
mounts are implicitly unmounted when the last process in the namespace
dies, meaning we don't need to try any funny business around cleaning up
processes in order to unmount.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/base/stagebase.py          | 30 ++------------
 targets/support/kill-chroot-pids.sh | 62 -----------------------------
 2 files changed, 4 insertions(+), 88 deletions(-)
 delete mode 100755 targets/support/kill-chroot-pids.sh

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index ec9a8f06..5fc11eae 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -638,17 +638,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
             assert self.settings[verify] == "blake2"
             self.settings.setdefault("gk_mainargs", []).append("--b2sum")
 
-    def kill_chroot_pids(self):
-        log.info('Checking for processes running in chroot and killing them.')
-
-        # Force environment variables to be exported so script can see them
-        self.setup_environment()
-
-        killcmd = normpath(self.settings["sharedir"] +
-                           self.settings["shdir"] + "/support/kill-chroot-pids.sh")
-        if os.path.exists(killcmd):
-            cmd([killcmd], env=self.env)
-
     def mount_safety_check(self):
         """
         Check and verify that none of our paths in mypath are mounted. We don't
@@ -920,18 +909,10 @@ class StageBase(TargetBase, ClearBase, GenBase):
             try:
                 cxt = libmount.Context(target=str(target))
                 cxt.umount()
-            except OSError:
-                log.warning('First attempt to unmount failed: %s', target)
-                log.warning('Killing any pids still running in the chroot')
-
-                self.kill_chroot_pids()
-
-                try:
-                    cxt.umount()
-                except OSError as e:
-                    umount_failed = True
-                    log.warning("Couldn't umount: %s, %s", target,
-                                e.strerror)
+            except OSError as e:
+                log.warning("Couldn't umount: %s, %s", target,
+                            e.strerror)
+                umount_failed = True
 
         if umount_failed:
             # if any bind mounts really failed, then we need to raise
@@ -1382,9 +1363,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
     def run(self):
         self.chroot_lock.write_lock()
 
-        # Kill any pids in the chroot
-        self.kill_chroot_pids()
-
         # Check for mounts right away and abort if we cannot unmount them
         self.mount_safety_check()
 
diff --git a/targets/support/kill-chroot-pids.sh b/targets/support/kill-chroot-pids.sh
deleted file mode 100755
index ea8ee402..00000000
--- a/targets/support/kill-chroot-pids.sh
+++ /dev/null
@@ -1,62 +0,0 @@
-#!/bin/bash
-# Script to kill processes found running in the chroot.
-
-if [ "${clst_chroot_path}" == "/" ]
-then
-	echo "Aborting .... clst_chroot_path is set to /"
-	echo "This is very dangerous"
-	exit 1
-fi
-
-if [ "${clst_chroot_path}" == "" ]
-then
-	echo "Aborting .... clst_chroot_path is NOT set"
-	echo "This is very dangerous"
-	exit 1
-fi
-
-j=0
-declare -a pids
-# Get files and dirs in /proc
-for i in `ls /proc`
-do
-	# Test for directories
-	if [ -d /proc/$i ]
-	then
-	# Search for exe containing string inside ${clst_chroot_path}
-	ls -la --color=never /proc/$i 2>&1 |grep exe|grep ${clst_chroot_path} > /dev/null
-
-	# If found
-	if [ $? == 0 ]
-	then
-		# Assign the pid into the pids array
-		pids[$j]=$i
-		j=$(($j+1))
-	fi
-	fi
-done
-
-if [ ${j} -gt 0 ]
-then
-	echo
-	echo "Killing process(es)"
-	echo "pid: process name"
-	for pid in ${pids[@]}
-	do
-		P_NAME=$(ls -la --color=never /proc/${pid} 2>&1 |grep exe|grep ${clst_chroot_path}|awk '{print $11}')
-		echo ${pid}: ${P_NAME}
-	done
-	echo
-	echo "Press Ctrl-C within 10 seconds to abort"
-
-	sleep 10
-
-	for pid in ${pids[@]}
-	do
-		kill -9 ${pid}
-	done
-
-	# Small sleep here to give the process(es) a chance to die before running unbind again.
-	sleep 5
-
-fi
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 11/12] catalyst: Remove mount_safety_check()
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
                   ` (8 preceding siblings ...)
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 10/12] catalyst: Remove kill_support_pids() Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 12/12] catalyst: Drop unbind() Matt Turner
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

mount_safety_check() exists to prevent bad things from happening if a
previous catalyst invocation left bind mounts active in the chroot.
E.g., a previous catalyst invocation is interrupted without unmounting
the bind mounts. A new catalyst invocation runs and cleans the old
chroot, which inadvertently deletes files outside of the chroot via the
bind mounts.

With all the mounts now inside a namespace, it is no longer possible to
have mounts accessible outside the build sequence. In fact, I think this
code has been unnecessary since commit e5a53e42 ("catalyst: create
namespaces for building").

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/base/stagebase.py | 37 -------------------------------------
 1 file changed, 37 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index 5fc11eae..d5454b77 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -638,39 +638,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
             assert self.settings[verify] == "blake2"
             self.settings.setdefault("gk_mainargs", []).append("--b2sum")
 
-    def mount_safety_check(self):
-        """
-        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(self.settings["chroot_path"]):
-            return
-
-        log.debug('self.mount = %s', self.mount)
-        for x in [x for x in self.mount if self.mount[x]['enable']]:
-            target = normpath(self.settings['chroot_path'] +
-                              self.mount[x]['target'])
-            log.debug('mount_safety_check() x = %s %s', x, target)
-            if not os.path.exists(target):
-                continue
-
-            if ismount(target):
-                # Something is still mounted
-                try:
-                    log.warning(
-                        '%s is still mounted; performing auto-bind-umount...', target)
-                    # Try to umount stuff ourselves
-                    self.unbind()
-                    if ismount(target):
-                        raise CatalystError("Auto-unbind failed for " + target)
-                    log.notice('Auto-unbind successful...')
-                except CatalystError:
-                    raise CatalystError("Unable to auto-unbind " + target)
-
     def unpack(self):
-
         clst_unpack_hash = self.resume.get("unpack")
 
         # Set up all unpack info settings
@@ -755,8 +723,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
                     % self.settings["source_path"])
 
         if _unpack:
-            self.mount_safety_check()
-
             if invalid_chroot:
                 if "autoresume" in self.settings["options"]:
                     log.notice(
@@ -1363,9 +1329,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
     def run(self):
         self.chroot_lock.write_lock()
 
-        # Check for mounts right away and abort if we cannot unmount them
-        self.mount_safety_check()
-
         if "clear-autoresume" in self.settings["options"]:
             self.clear_autoresume()
 
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [gentoo-catalyst] [PATCH 12/12] catalyst: Drop unbind()
  2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
                   ` (9 preceding siblings ...)
  2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 11/12] catalyst: Remove mount_safety_check() Matt Turner
@ 2020-10-29 16:16 ` Matt Turner
  10 siblings, 0 replies; 12+ messages in thread
From: Matt Turner @ 2020-10-29 16:16 UTC (permalink / raw
  To: gentoo-catalyst; +Cc: Matt Turner

mount_namespaces(7) says

	A mount ceases to be a member of a peer group when either the
	mount is explicitly unmounted, or when the mount is implicitly
	unmounted because a mount namespace is removed (because it has
	no more member processes).

As a result, we can rely on exiting the mount namespace to unmount the
bind mounts.

Signed-off-by: Matt Turner <mattst88@gentoo.org>
---
 catalyst/base/stagebase.py        | 45 +------------------------------
 catalyst/targets/embedded.py      |  1 -
 catalyst/targets/livecd_stage1.py |  1 -
 catalyst/targets/livecd_stage2.py |  2 --
 catalyst/targets/netboot.py       |  3 ---
 catalyst/targets/stage4.py        |  1 -
 6 files changed, 1 insertion(+), 52 deletions(-)

diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
index d5454b77..a75dbdf9 100644
--- a/catalyst/base/stagebase.py
+++ b/catalyst/base/stagebase.py
@@ -498,7 +498,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
             "setup_environment",
             "run_local",
             "preclean",
-            "unbind",
         ])
         self.finish_sequence.extend([
             "clean",
@@ -853,40 +852,8 @@ class StageBase(TargetBase, ClearBase, GenBase):
                                        fstype=fstype, options=options)
                 cxt.mount()
             except OSError as e:
-                self.unbind()
                 raise CatalystError(f"Couldn't mount: {source}, {e.strerror}")
 
-    def unbind(self):
-        chroot_path = self.settings["chroot_path"]
-        umount_failed = False
-
-        # Unmount in reverse order
-        for target in [Path(chroot_path + self.mount[x]['target'])
-                       for x in reversed(self.mount)
-                       if self.mount[x]['enable']]:
-            if not target.exists():
-                log.debug('%s does not exist. Skipping', target)
-                continue
-
-            if not ismount(target):
-                log.debug('%s is not a mount point. Skipping', target)
-                continue
-
-            try:
-                cxt = libmount.Context(target=str(target))
-                cxt.umount()
-            except OSError as e:
-                log.warning("Couldn't umount: %s, %s", target,
-                            e.strerror)
-                umount_failed = True
-
-        if umount_failed:
-            # if any bind mounts really failed, then we need to raise
-            # this to potentially prevent an upcoming bash stage cleanup script
-            # from wiping our bind mounts.
-            raise CatalystError(
-                "Couldn't umount one or more bind-mounts; aborting for safety.")
-
     def chroot_setup(self):
         self.makeconf = read_makeconf(normpath(self.settings["chroot_path"] +
                                                self.settings["make_conf"]))
@@ -1190,7 +1157,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
                         env=self.env)
                     self.resume.enable("remove")
             except:
-                self.unbind()
                 raise
 
     def preclean(self):
@@ -1206,7 +1172,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
                 self.resume.enable("preclean")
 
         except:
-            self.unbind()
             raise CatalystError("Build failed, could not execute preclean")
 
     def capture(self):
@@ -1269,7 +1234,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
                          self.settings['controller_file'])
 
         except CatalystError:
-            self.unbind()
             raise CatalystError("Stage build aborting due to error.",
                                 print_traceback=False)
 
@@ -1349,7 +1313,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
 
         with namespace(mount=True):
             if not self.run_sequence(self.build_sequence):
-                self.unbind()
                 return False
 
         if not self.run_sequence(self.finish_sequence):
@@ -1375,7 +1338,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
                     env=self.env)
                 log.info('unmerge shell script')
             except CatalystError:
-                self.unbind()
                 raise
             self.resume.enable("unmerge")
 
@@ -1450,7 +1412,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
                     fileutils.touch(build_packages_resume)
                     self.resume.enable("build_packages")
                 except CatalystError:
-                    self.unbind()
                     raise CatalystError(
                         self.settings["spec_prefix"] +
                         "build aborting due to error.")
@@ -1474,7 +1435,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
                     self._build_kernel(kname=kname)
                 self.resume.enable("build_kernel")
             except CatalystError:
-                self.unbind()
                 raise CatalystError(
                     "build aborting due to kernel build error.",
                     print_traceback=True)
@@ -1518,7 +1478,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
         key = 'boot/kernel/' + kname + '/config'
         if key in self.settings:
             if not os.path.exists(self.settings[key]):
-                self.unbind()
                 raise CatalystError("Can't find kernel config: %s" %
                                     self.settings[key])
 
@@ -1527,7 +1486,7 @@ class StageBase(TargetBase, ClearBase, GenBase):
                             self.settings['chroot_path'] + '/var/tmp/' + kname + '.config')
 
             except IOError:
-                self.unbind()
+                raise
 
     def _copy_initramfs_overlay(self, kname):
         key = 'boot/kernel/' + kname + '/initramfs_overlay'
@@ -1557,7 +1516,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
                 env=self.env)
             self.resume.enable("bootloader")
         except CatalystError:
-            self.unbind()
             raise CatalystError("Script aborting due to error.")
 
     def livecd_update(self):
@@ -1573,7 +1531,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
             self.resume.enable("livecd_update")
 
         except CatalystError:
-            self.unbind()
             raise CatalystError(
                 "build aborting due to livecd_update error.")
 
diff --git a/catalyst/targets/embedded.py b/catalyst/targets/embedded.py
index 1314ce7c..e9138437 100644
--- a/catalyst/targets/embedded.py
+++ b/catalyst/targets/embedded.py
@@ -56,7 +56,6 @@ class embedded(StageBase):
             "root_overlay",
             "fsscript",
             "unmerge",
-            "unbind",
         ])
         self.finish_sequence.extend([
             "remove",
diff --git a/catalyst/targets/livecd_stage1.py b/catalyst/targets/livecd_stage1.py
index 81367053..5c5e9f58 100644
--- a/catalyst/targets/livecd_stage1.py
+++ b/catalyst/targets/livecd_stage1.py
@@ -34,7 +34,6 @@ class livecd_stage1(StageBase):
             "chroot_setup",
             "setup_environment",
             "build_packages",
-            "unbind",
         ])
         self.finish_sequence.extend([
             "clean",
diff --git a/catalyst/targets/livecd_stage2.py b/catalyst/targets/livecd_stage2.py
index f6c14919..3606047f 100644
--- a/catalyst/targets/livecd_stage2.py
+++ b/catalyst/targets/livecd_stage2.py
@@ -80,7 +80,6 @@ class livecd_stage2(StageBase):
                     for x in self.settings["livecd/modblacklist"]:
                         myf.write("\nblacklist "+x)
             except:
-                self.unbind()
                 raise CatalystError("Couldn't open " +
                                     self.settings["chroot_path"] +
                                     "/etc/modprobe.d/blacklist.conf.",
@@ -109,7 +108,6 @@ class livecd_stage2(StageBase):
                 "fsscript",
                 "rcupdate",
                 "unmerge",
-                "unbind",
             ])
             self.finish_sequence.extend([
                 "remove",
diff --git a/catalyst/targets/netboot.py b/catalyst/targets/netboot.py
index 9a0a4156..55f4dff1 100644
--- a/catalyst/targets/netboot.py
+++ b/catalyst/targets/netboot.py
@@ -93,7 +93,6 @@ class netboot(StageBase):
                 cmd([self.settings['controller_file'], 'image'] +
                     myfiles, env=self.env)
             except CatalystError:
-                self.unbind()
                 raise CatalystError("Failed to copy files to image!",
                                     print_traceback=True)
 
@@ -121,7 +120,6 @@ class netboot(StageBase):
             cmd([self.settings['controller_file'], 'final'], env=self.env)
             log.notice('Netboot Build Finished!')
         except CatalystError:
-            self.unbind()
             raise CatalystError("Failed to move kernel images!",
                                 print_traceback=True)
 
@@ -178,7 +176,6 @@ class netboot(StageBase):
             "move_kernels",
             "remove",
             "empty",
-            "unbind",
         ])
         self.finish_sequence.extend([
             "clean",
diff --git a/catalyst/targets/stage4.py b/catalyst/targets/stage4.py
index 78a5c780..b7f74b01 100644
--- a/catalyst/targets/stage4.py
+++ b/catalyst/targets/stage4.py
@@ -57,7 +57,6 @@ class stage4(StageBase):
             "preclean",
             "rcupdate",
             "unmerge",
-            "unbind",
         ])
         self.finish_sequence.extend([
             "remove",
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-10-29 16:16 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-29 16:16 [gentoo-catalyst] [PATCH 01/12] catalyst: Replace pathcompare() Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 02/12] catalyst: Rewrite ismount() to use libmount Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 03/12] catalyst: Use libmount for handling mounts Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 04/12] catalyst: Move action_sequence out of self.settings[] Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 05/12] catalyst: Use .extend() and .append() for action_sequence Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 06/12] catalyst: Split action_sequence into prepare/build/finish Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 07/12] catalyst: Factor out run_sequence() Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 08/12] catalyst: Add and use namespace context manager Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 09/12] catalyst: Run the build sequence in new mount namespace Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 10/12] catalyst: Remove kill_support_pids() Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 11/12] catalyst: Remove mount_safety_check() Matt Turner
2020-10-29 16:16 ` [gentoo-catalyst] [PATCH 12/12] catalyst: Drop unbind() Matt Turner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox