[PATCH v1 0/5] Restore downstream mounts compatibility

[PATCH v1 0/5] Restore downstream mounts compatibility

[Anton Mikanovich]

Revert commit d21d49578e5a3b0019075d1946bd93a95914fcca which has broken
compatibility with upstream projects.
Make the same fix by catching bb.build.TaskFailed event and unmount
any lost paths.
Also make undo_mounts functions to be safe for calling in case there is
no do_mounts call before. This will make fail policy more tolerant.
Enable refrence counting for image mounts in case there are several
images using like ubi-ubifs-img does.

This implementation do not call buildchroot_undo_mounts in task fail
handler, because there is hard not to broke other tasks execution. So
it will not fully cover pure buildchroot_do_mounts usage like
wic-img.bbclass does.

Tested with kas-iot2050-example project from:
https://github.com/siemens/meta-iot2050

Anton Mikanovich (5):
  Revert "dpkg: Make mount buildroot reliable"
  mount: Allow calling unmount on not mounted paths
  dpkg: Remove unmount loop
  image: Add reference counter
  events: Unmount all lost mounts at task fail

 meta/classes/buildchroot.bbclass |  20 ++---
 meta/classes/dpkg-base.bbclass   | 122 +++++++++++--------------------
 meta/classes/dpkg-gbp.bbclass    |   8 +-
 meta/classes/dpkg.bbclass        |  14 +---
 meta/classes/image.bbclass       |  39 +++++++++-
 meta/classes/isar-events.bbclass |  27 +++++++
 meta/classes/rootfs.bbclass      |  20 ++---
 7 files changed, 136 insertions(+), 114 deletions(-)

-- 
2.25.1

[PATCH v1 1/5] Revert "dpkg: Make mount buildroot reliable"

[Anton Mikanovich]

This reverts commit d21d49578e5a3b0019075d1946bd93a95914fcca.
---
 meta/classes/dpkg-base.bbclass | 127 +++++++++++++--------------------
 meta/classes/dpkg-gbp.bbclass  |   8 ++-
 meta/classes/dpkg.bbclass      |  14 ++--
 3 files changed, 59 insertions(+), 90 deletions(-)

diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg-base.bbclass
index ed162b3..97661a6 100644
--- a/meta/classes/dpkg-base.bbclass
+++ b/meta/classes/dpkg-base.bbclass
@@ -61,7 +61,12 @@ addtask patch before do_adjust_git
 
 SRC_APT ?= ""
 
-fetch_apt() {
+do_apt_fetch() {
+    if [ -z "${@d.getVar("SRC_APT", True).strip()}" ]; then
+        return 0
+    fi
+    dpkg_do_mounts
+    E="${@ isar_export_proxies(d)}"
     sudo -E chroot ${BUILDCHROOT_DIR} /usr/bin/apt-get update \
         -o Dir::Etc::SourceList="sources.list.d/isar-apt.list" \
         -o Dir::Etc::SourceParts="-" \
@@ -71,25 +76,21 @@ fetch_apt() {
         sudo -E chroot --userspec=$( id -u ):$( id -g ) ${BUILDCHROOT_DIR} \
             sh -c 'mkdir -p /downloads/deb-src/"$1"/"$2" && cd /downloads/deb-src/"$1"/"$2" && apt-get -y --download-only --only-source source "$2"' my_script "${DISTRO}" "${uri}"
     done
-}
-
-python do_apt_fetch() {
-    src_apt = d.getVar("SRC_APT", True)
-    if not src_apt:
-        return 0
 
-    dpkg_do_mounts(d)
-    try:
-        isar_export_proxies(d)
-        bb.build.exec_func("fetch_apt", d)
-    finally:
-        dpkg_undo_mounts(d)
+    dpkg_undo_mounts
 }
 
 addtask apt_fetch after do_unpack before do_apt_unpack
 do_apt_fetch[lockfiles] += "${REPO_ISAR_DIR}/isar.lock"
 
-unpack_apt() {
+do_apt_unpack() {
+    if [ -z "${@d.getVar("SRC_APT", True).strip()}" ]; then
+        return 0
+    fi
+    rm -rf ${S}
+    dpkg_do_mounts
+    E="${@ isar_export_proxies(d)}"
+
     for uri in "${SRC_APT}"; do
         sudo -E chroot --userspec=$( id -u ):$( id -g ) ${BUILDCHROOT_DIR} \
             sh -c ' \
@@ -100,25 +101,8 @@ unpack_apt() {
                 dpkg-source -x "${dscfile}" "${PPS}"' \
                     my_script "${DISTRO}" "${uri}"
     done
-}
 
-python do_apt_unpack() {
-    import shutil
-
-    src_apt = d.getVar("SRC_APT", True)
-    if not src_apt:
-        return 0
-
-    srcsubdir = d.getVar('S', True)
-    if os.path.exists(srcsubdir):
-        shutil.rmtree(srcsubdir)
-
-    dpkg_do_mounts(d)
-    try:
-        isar_export_proxies(d)
-        bb.build.exec_func("unpack_apt", d)
-    finally:
-        dpkg_undo_mounts(d)
+    dpkg_undo_mounts
 }
 
 addtask apt_unpack after do_apt_fetch before do_patch
@@ -162,38 +146,25 @@ do_prepare_build[deptask] = "do_deploy_deb"
 
 BUILDROOT = "${BUILDCHROOT_DIR}/${PP}"
 
-def ismount(path):
-    real = os.path.realpath(path)
-    with open('/proc/mounts') as f:
-        for line in f.readlines():
-            if len(line.split()) > 2 and real == line.split()[1]:
-                return True
-    return False
-
-def dpkg_do_mounts(d):
-    buildroot = d.getVar('BUILDROOT', True)
-    if ismount(buildroot):
-        bb.warn('Path %s already mounted!' % buildroot)
-        return
-    workdir = d.getVar('WORKDIR', True)
-    os.makedirs(buildroot, exist_ok=True)
-    os.system('sudo mount --bind %s %s' % (workdir, buildroot))
-    bb.build.exec_func("buildchroot_do_mounts", d)
-
-def dpkg_undo_mounts(d):
-    bb.build.exec_func("buildchroot_undo_mounts", d)
-    buildroot = d.getVar('BUILDROOT', True)
-    if not ismount(buildroot):
-        bb.warn('Path %s not mounted!' % buildroot)
-        return
-    for i in range(200):
-        if not os.system('sudo umount %s' % buildroot):
-            os.rmdir(buildroot)
-            return
-        if i % 100 == 0:
-            bb.warn("%s: Couldn't unmount, retrying..." % buildroot)
-        time.sleep(0.1)
-    bb.fatal("Couldn't unmount, exiting...")
+dpkg_do_mounts() {
+    mkdir -p ${BUILDROOT}
+    sudo mount --bind ${WORKDIR} ${BUILDROOT}
+
+    buildchroot_do_mounts
+}
+
+dpkg_undo_mounts() {
+    i=1
+    while ! sudo umount ${BUILDROOT}; do
+        sleep 0.1
+        i=`expr $i + 1`
+        if [ $i -gt 100 ]; then
+            bbwarn "${BUILDROOT}: Couldn't unmount, retrying..."
+            i=1
+        fi
+    done
+    sudo rmdir ${BUILDROOT}
+}
 
 # Placeholder for actual dpkg_runbuild() implementation
 dpkg_runbuild() {
@@ -203,12 +174,10 @@ dpkg_runbuild() {
 python do_dpkg_build() {
     lock = bb.utils.lockfile(d.getVar("REPO_ISAR_DIR") + "/isar.lock",
                              shared=True)
-    dpkg_do_mounts(d)
-    try:
-        bb.build.exec_func("dpkg_runbuild", d)
-    finally:
-        dpkg_undo_mounts(d)
-        bb.utils.unlockfile(lock)
+    bb.build.exec_func("dpkg_do_mounts", d)
+    bb.build.exec_func("dpkg_runbuild", d)
+    bb.build.exec_func("dpkg_undo_mounts", d)
+    bb.utils.unlockfile(lock)
 }
 
 addtask dpkg_build before do_build
@@ -252,16 +221,16 @@ python do_devshell() {
     oe_lib_path = os.path.join(d.getVar('LAYERDIR_core'), 'lib')
     sys.path.insert(0, oe_lib_path)
 
-    dpkg_do_mounts(d)
-    try:
-        isar_export_proxies(d)
+    bb.build.exec_func('dpkg_do_mounts', d)
+
+    isar_export_proxies(d)
+
+    buildchroot = d.getVar('BUILDCHROOT_DIR')
+    pp_pps = os.path.join(d.getVar('PP'), d.getVar('PPS'))
+    termcmd = "sudo -E chroot {0} sh -c 'cd {1}; $SHELL -i'"
+    oe_terminal(termcmd.format(buildchroot, pp_pps), "Isar devshell", d)
 
-        buildchroot = d.getVar('BUILDCHROOT_DIR')
-        pp_pps = os.path.join(d.getVar('PP'), d.getVar('PPS'))
-        termcmd = "sudo -E chroot {0} sh -c 'cd {1}; $SHELL -i'"
-        oe_terminal(termcmd.format(buildchroot, pp_pps), "Isar devshell", d)
-    finally:
-        dpkg_undo_mounts(d)
+    bb.build.exec_func('dpkg_undo_mounts', d)
 }
 
 addtask devshell after do_prepare_build
diff --git a/meta/classes/dpkg-gbp.bbclass b/meta/classes/dpkg-gbp.bbclass
index 20d2d4c..d956e8c 100644
--- a/meta/classes/dpkg-gbp.bbclass
+++ b/meta/classes/dpkg-gbp.bbclass
@@ -12,7 +12,12 @@ PATCHTOOL ?= "git"
 GBP_DEPENDS ?= "git-buildpackage pristine-tar"
 GBP_EXTRA_OPTIONS ?= "--git-pristine-tar"
 
-builddeps_install_append() {
+do_install_builddeps_append() {
+    dpkg_do_mounts
+    distro="${DISTRO}"
+    if [ ${ISAR_CROSS_COMPILE} -eq 1 ]; then
+       distro="${HOST_DISTRO}"
+    fi
     deb_dl_dir_import "${BUILDCHROOT_DIR}" "${distro}"
     sudo -E chroot ${BUILDCHROOT_DIR} \
         apt-get install -y -o Debug::pkgProblemResolver=yes \
@@ -21,6 +26,7 @@ builddeps_install_append() {
     sudo -E chroot ${BUILDCHROOT_DIR} \
         apt-get install -y -o Debug::pkgProblemResolver=yes \
                         --no-install-recommends ${GBP_DEPENDS}
+    dpkg_undo_mounts
 }
 
 dpkg_runbuild_prepend() {
diff --git a/meta/classes/dpkg.bbclass b/meta/classes/dpkg.bbclass
index 29e2b89..4e7c2f7 100644
--- a/meta/classes/dpkg.bbclass
+++ b/meta/classes/dpkg.bbclass
@@ -6,7 +6,9 @@ inherit dpkg-base
 PACKAGE_ARCH ?= "${DISTRO_ARCH}"
 
 # Install build dependencies for package
-builddeps_install() {
+do_install_builddeps() {
+    dpkg_do_mounts
+    E="${@ isar_export_proxies(d)}"
     distro="${DISTRO}"
     if [ ${ISAR_CROSS_COMPILE} -eq 1 ]; then
        distro="${HOST_DISTRO}"
@@ -17,15 +19,7 @@ builddeps_install() {
     deb_dl_dir_export "${BUILDCHROOT_DIR}" "${distro}"
     sudo -E chroot ${BUILDCHROOT_DIR} /isar/deps.sh \
         ${PP}/${PPS} ${PACKAGE_ARCH}
-}
-
-python do_install_builddeps() {
-    dpkg_do_mounts(d)
-    isar_export_proxies(d)
-    try:
-        bb.build.exec_func("builddeps_install", d)
-    finally:
-        dpkg_undo_mounts(d)
+    dpkg_undo_mounts
 }
 
 addtask install_builddeps after do_prepare_build before do_dpkg_build
-- 
2.25.1

Re: [PATCH v1 1/5] Revert "dpkg: Make mount buildroot reliable"

[Jan Kiszka]

On 07.07.21 18:38, Anton Mikanovich wrote:
> This reverts commit d21d49578e5a3b0019075d1946bd93a95914fcca.

Signed-off and a short reasoning is missing.

Jan

> ---
>  meta/classes/dpkg-base.bbclass | 127 +++++++++++++--------------------
>  meta/classes/dpkg-gbp.bbclass  |   8 ++-
>  meta/classes/dpkg.bbclass      |  14 ++--
>  3 files changed, 59 insertions(+), 90 deletions(-)
> 
> diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg-base.bbclass
> index ed162b3..97661a6 100644
> --- a/meta/classes/dpkg-base.bbclass
> +++ b/meta/classes/dpkg-base.bbclass
> @@ -61,7 +61,12 @@ addtask patch before do_adjust_git
>  
>  SRC_APT ?= ""
>  
> -fetch_apt() {
> +do_apt_fetch() {
> +    if [ -z "${@d.getVar("SRC_APT", True).strip()}" ]; then
> +        return 0
> +    fi
> +    dpkg_do_mounts
> +    E="${@ isar_export_proxies(d)}"
>      sudo -E chroot ${BUILDCHROOT_DIR} /usr/bin/apt-get update \
>          -o Dir::Etc::SourceList="sources.list.d/isar-apt.list" \
>          -o Dir::Etc::SourceParts="-" \
> @@ -71,25 +76,21 @@ fetch_apt() {
>          sudo -E chroot --userspec=$( id -u ):$( id -g ) ${BUILDCHROOT_DIR} \
>              sh -c 'mkdir -p /downloads/deb-src/"$1"/"$2" && cd /downloads/deb-src/"$1"/"$2" && apt-get -y --download-only --only-source source "$2"' my_script "${DISTRO}" "${uri}"
>      done
> -}
> -
> -python do_apt_fetch() {
> -    src_apt = d.getVar("SRC_APT", True)
> -    if not src_apt:
> -        return 0
>  
> -    dpkg_do_mounts(d)
> -    try:
> -        isar_export_proxies(d)
> -        bb.build.exec_func("fetch_apt", d)
> -    finally:
> -        dpkg_undo_mounts(d)
> +    dpkg_undo_mounts
>  }
>  
>  addtask apt_fetch after do_unpack before do_apt_unpack
>  do_apt_fetch[lockfiles] += "${REPO_ISAR_DIR}/isar.lock"
>  
> -unpack_apt() {
> +do_apt_unpack() {
> +    if [ -z "${@d.getVar("SRC_APT", True).strip()}" ]; then
> +        return 0
> +    fi
> +    rm -rf ${S}
> +    dpkg_do_mounts
> +    E="${@ isar_export_proxies(d)}"
> +
>      for uri in "${SRC_APT}"; do
>          sudo -E chroot --userspec=$( id -u ):$( id -g ) ${BUILDCHROOT_DIR} \
>              sh -c ' \
> @@ -100,25 +101,8 @@ unpack_apt() {
>                  dpkg-source -x "${dscfile}" "${PPS}"' \
>                      my_script "${DISTRO}" "${uri}"
>      done
> -}
>  
> -python do_apt_unpack() {
> -    import shutil
> -
> -    src_apt = d.getVar("SRC_APT", True)
> -    if not src_apt:
> -        return 0
> -
> -    srcsubdir = d.getVar('S', True)
> -    if os.path.exists(srcsubdir):
> -        shutil.rmtree(srcsubdir)
> -
> -    dpkg_do_mounts(d)
> -    try:
> -        isar_export_proxies(d)
> -        bb.build.exec_func("unpack_apt", d)
> -    finally:
> -        dpkg_undo_mounts(d)
> +    dpkg_undo_mounts
>  }
>  
>  addtask apt_unpack after do_apt_fetch before do_patch
> @@ -162,38 +146,25 @@ do_prepare_build[deptask] = "do_deploy_deb"
>  
>  BUILDROOT = "${BUILDCHROOT_DIR}/${PP}"
>  
> -def ismount(path):
> -    real = os.path.realpath(path)
> -    with open('/proc/mounts') as f:
> -        for line in f.readlines():
> -            if len(line.split()) > 2 and real == line.split()[1]:
> -                return True
> -    return False
> -
> -def dpkg_do_mounts(d):
> -    buildroot = d.getVar('BUILDROOT', True)
> -    if ismount(buildroot):
> -        bb.warn('Path %s already mounted!' % buildroot)
> -        return
> -    workdir = d.getVar('WORKDIR', True)
> -    os.makedirs(buildroot, exist_ok=True)
> -    os.system('sudo mount --bind %s %s' % (workdir, buildroot))
> -    bb.build.exec_func("buildchroot_do_mounts", d)
> -
> -def dpkg_undo_mounts(d):
> -    bb.build.exec_func("buildchroot_undo_mounts", d)
> -    buildroot = d.getVar('BUILDROOT', True)
> -    if not ismount(buildroot):
> -        bb.warn('Path %s not mounted!' % buildroot)
> -        return
> -    for i in range(200):
> -        if not os.system('sudo umount %s' % buildroot):
> -            os.rmdir(buildroot)
> -            return
> -        if i % 100 == 0:
> -            bb.warn("%s: Couldn't unmount, retrying..." % buildroot)
> -        time.sleep(0.1)
> -    bb.fatal("Couldn't unmount, exiting...")
> +dpkg_do_mounts() {
> +    mkdir -p ${BUILDROOT}
> +    sudo mount --bind ${WORKDIR} ${BUILDROOT}
> +
> +    buildchroot_do_mounts
> +}
> +
> +dpkg_undo_mounts() {
> +    i=1
> +    while ! sudo umount ${BUILDROOT}; do
> +        sleep 0.1
> +        i=`expr $i + 1`
> +        if [ $i -gt 100 ]; then
> +            bbwarn "${BUILDROOT}: Couldn't unmount, retrying..."
> +            i=1
> +        fi
> +    done
> +    sudo rmdir ${BUILDROOT}
> +}
>  
>  # Placeholder for actual dpkg_runbuild() implementation
>  dpkg_runbuild() {
> @@ -203,12 +174,10 @@ dpkg_runbuild() {
>  python do_dpkg_build() {
>      lock = bb.utils.lockfile(d.getVar("REPO_ISAR_DIR") + "/isar.lock",
>                               shared=True)
> -    dpkg_do_mounts(d)
> -    try:
> -        bb.build.exec_func("dpkg_runbuild", d)
> -    finally:
> -        dpkg_undo_mounts(d)
> -        bb.utils.unlockfile(lock)
> +    bb.build.exec_func("dpkg_do_mounts", d)
> +    bb.build.exec_func("dpkg_runbuild", d)
> +    bb.build.exec_func("dpkg_undo_mounts", d)
> +    bb.utils.unlockfile(lock)
>  }
>  
>  addtask dpkg_build before do_build
> @@ -252,16 +221,16 @@ python do_devshell() {
>      oe_lib_path = os.path.join(d.getVar('LAYERDIR_core'), 'lib')
>      sys.path.insert(0, oe_lib_path)
>  
> -    dpkg_do_mounts(d)
> -    try:
> -        isar_export_proxies(d)
> +    bb.build.exec_func('dpkg_do_mounts', d)
> +
> +    isar_export_proxies(d)
> +
> +    buildchroot = d.getVar('BUILDCHROOT_DIR')
> +    pp_pps = os.path.join(d.getVar('PP'), d.getVar('PPS'))
> +    termcmd = "sudo -E chroot {0} sh -c 'cd {1}; $SHELL -i'"
> +    oe_terminal(termcmd.format(buildchroot, pp_pps), "Isar devshell", d)
>  
> -        buildchroot = d.getVar('BUILDCHROOT_DIR')
> -        pp_pps = os.path.join(d.getVar('PP'), d.getVar('PPS'))
> -        termcmd = "sudo -E chroot {0} sh -c 'cd {1}; $SHELL -i'"
> -        oe_terminal(termcmd.format(buildchroot, pp_pps), "Isar devshell", d)
> -    finally:
> -        dpkg_undo_mounts(d)
> +    bb.build.exec_func('dpkg_undo_mounts', d)
>  }
>  
>  addtask devshell after do_prepare_build
> diff --git a/meta/classes/dpkg-gbp.bbclass b/meta/classes/dpkg-gbp.bbclass
> index 20d2d4c..d956e8c 100644
> --- a/meta/classes/dpkg-gbp.bbclass
> +++ b/meta/classes/dpkg-gbp.bbclass
> @@ -12,7 +12,12 @@ PATCHTOOL ?= "git"
>  GBP_DEPENDS ?= "git-buildpackage pristine-tar"
>  GBP_EXTRA_OPTIONS ?= "--git-pristine-tar"
>  
> -builddeps_install_append() {
> +do_install_builddeps_append() {
> +    dpkg_do_mounts
> +    distro="${DISTRO}"
> +    if [ ${ISAR_CROSS_COMPILE} -eq 1 ]; then
> +       distro="${HOST_DISTRO}"
> +    fi
>      deb_dl_dir_import "${BUILDCHROOT_DIR}" "${distro}"
>      sudo -E chroot ${BUILDCHROOT_DIR} \
>          apt-get install -y -o Debug::pkgProblemResolver=yes \
> @@ -21,6 +26,7 @@ builddeps_install_append() {
>      sudo -E chroot ${BUILDCHROOT_DIR} \
>          apt-get install -y -o Debug::pkgProblemResolver=yes \
>                          --no-install-recommends ${GBP_DEPENDS}
> +    dpkg_undo_mounts
>  }
>  
>  dpkg_runbuild_prepend() {
> diff --git a/meta/classes/dpkg.bbclass b/meta/classes/dpkg.bbclass
> index 29e2b89..4e7c2f7 100644
> --- a/meta/classes/dpkg.bbclass
> +++ b/meta/classes/dpkg.bbclass
> @@ -6,7 +6,9 @@ inherit dpkg-base
>  PACKAGE_ARCH ?= "${DISTRO_ARCH}"
>  
>  # Install build dependencies for package
> -builddeps_install() {
> +do_install_builddeps() {
> +    dpkg_do_mounts
> +    E="${@ isar_export_proxies(d)}"
>      distro="${DISTRO}"
>      if [ ${ISAR_CROSS_COMPILE} -eq 1 ]; then
>         distro="${HOST_DISTRO}"
> @@ -17,15 +19,7 @@ builddeps_install() {
>      deb_dl_dir_export "${BUILDCHROOT_DIR}" "${distro}"
>      sudo -E chroot ${BUILDCHROOT_DIR} /isar/deps.sh \
>          ${PP}/${PPS} ${PACKAGE_ARCH}
> -}
> -
> -python do_install_builddeps() {
> -    dpkg_do_mounts(d)
> -    isar_export_proxies(d)
> -    try:
> -        bb.build.exec_func("builddeps_install", d)
> -    finally:
> -        dpkg_undo_mounts(d)
> +    dpkg_undo_mounts
>  }
>  
>  addtask install_builddeps after do_prepare_build before do_dpkg_build
> 

[PATCH v1 2/5] mount: Allow calling unmount on not mounted paths

[Anton Mikanovich]

Make buildchroot_undo_mounts and rootfs_undo_mounts functions do not
fail if calling without buildchroot_do_mounts and rootfs_do_mounts.

Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
---
 meta/classes/buildchroot.bbclass | 20 ++++++++++----------
 meta/classes/rootfs.bbclass      | 20 ++++++++++----------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/meta/classes/buildchroot.bbclass b/meta/classes/buildchroot.bbclass
index 806a29f..9c27711 100644
--- a/meta/classes/buildchroot.bbclass
+++ b/meta/classes/buildchroot.bbclass
@@ -23,16 +23,18 @@ python __anonymous() {
 
 MOUNT_LOCKFILE = "${BUILDCHROOT_DIR}.lock"
 
+BUILDCHROOT_MOUNT_CNT = "${BUILDCHROOT_DIR}.mount"
+
 buildchroot_do_mounts() {
     sudo -s <<'EOSUDO'
         ( flock 9
         set -e
 
         count="1"
-        if [ -f '${BUILDCHROOT_DIR}.mount' ]; then
-            count=$(($(cat '${BUILDCHROOT_DIR}.mount') + 1))
+        if [ -f "${BUILDCHROOT_MOUNT_CNT}" ]; then
+            count=$(($(cat "${BUILDCHROOT_MOUNT_CNT}") + 1))
         fi
-        echo $count > '${BUILDCHROOT_DIR}.mount'
+        echo $count > "${BUILDCHROOT_MOUNT_CNT}"
         if [ $count -gt 1 ]; then
             exit 0
         fi
@@ -77,17 +79,15 @@ buildchroot_undo_mounts() {
         ( flock 9
         set -e
 
-        if [ -f '${BUILDCHROOT_DIR}.mount' ]; then
-            count=$(($(cat '${BUILDCHROOT_DIR}.mount') - 1))
-            echo $count > '${BUILDCHROOT_DIR}.mount'
-        else
-            echo "Could not find mount counter"
-            exit 1
+        count="0"
+        if [ -f "${BUILDCHROOT_MOUNT_CNT}" ]; then
+            count=$(($(cat "${BUILDCHROOT_MOUNT_CNT}") - 1))
+            echo $count > "${BUILDCHROOT_MOUNT_CNT}"
         fi
         if [ $count -gt 0 ]; then
             exit 0
         fi
-        rm ${BUILDCHROOT_DIR}.mount
+        rm -f "${BUILDCHROOT_MOUNT_CNT}"
 
         mountpoint -q '${BUILDCHROOT_DIR}/base-apt' && \
             umount ${BUILDCHROOT_DIR}/base-apt && \
diff --git a/meta/classes/rootfs.bbclass b/meta/classes/rootfs.bbclass
index 50d6408..9ccc8e3 100644
--- a/meta/classes/rootfs.bbclass
+++ b/meta/classes/rootfs.bbclass
@@ -28,6 +28,8 @@ export LC_ALL = "C"
 
 MOUNT_LOCKFILE = "${ROOTFSDIR}.lock"
 
+ROOTFS_MOUNT_CNT = "${ROOTFSDIR}.mount"
+
 rootfs_do_mounts[weight] = "3"
 rootfs_do_mounts() {
     sudo -s <<'EOSUDO'
@@ -35,10 +37,10 @@ rootfs_do_mounts() {
         set -e
 
         count="1"
-        if [ -f '${ROOTFSDIR}.mount' ]; then
-            count=$(($(cat '${ROOTFSDIR}.mount') + 1))
+        if [ -f "${ROOTFS_MOUNT_CNT}" ]; then
+            count=$(($(cat "${ROOTFS_MOUNT_CNT}") + 1))
         fi
-        echo $count > '${ROOTFSDIR}.mount'
+        echo $count > "${ROOTFS_MOUNT_CNT}"
         if [ $count -gt 1 ]; then
             exit 0
         fi
@@ -79,17 +81,15 @@ rootfs_undo_mounts() {
         ( flock 9
         set -e
 
-        if [ -f '${ROOTFSDIR}.mount' ]; then
-            count=$(($(cat '${ROOTFSDIR}.mount') - 1))
-            echo $count > '${ROOTFSDIR}.mount'
-        else
-            echo "Could not find mount counter"
-            exit 1
+        count="0"
+        if [ -f "${ROOTFS_MOUNT_CNT}" ]; then
+            count=$(($(cat "${ROOTFS_MOUNT_CNT}") - 1))
+            echo $count > "${ROOTFS_MOUNT_CNT}"
         fi
         if [ $count -gt 0 ]; then
             exit 0
         fi
-        rm ${ROOTFSDIR}.mount
+        rm -f "${ROOTFS_MOUNT_CNT}"
 
         mountpoint -q '${ROOTFSDIR}/base-apt' && \
             umount ${ROOTFSDIR}/base-apt && \
-- 
2.25.1

Re: [PATCH v1 2/5] mount: Allow calling unmount on not mounted paths

[Jan Kiszka]

On 07.07.21 18:38, Anton Mikanovich wrote:
> Make buildchroot_undo_mounts and rootfs_undo_mounts functions do not
> fail if calling without buildchroot_do_mounts and rootfs_do_mounts.
> 

"Why" is missing, you only have the "what" here.

Jan

> Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
> ---
>  meta/classes/buildchroot.bbclass | 20 ++++++++++----------
>  meta/classes/rootfs.bbclass      | 20 ++++++++++----------
>  2 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/meta/classes/buildchroot.bbclass b/meta/classes/buildchroot.bbclass
> index 806a29f..9c27711 100644
> --- a/meta/classes/buildchroot.bbclass
> +++ b/meta/classes/buildchroot.bbclass
> @@ -23,16 +23,18 @@ python __anonymous() {
>  
>  MOUNT_LOCKFILE = "${BUILDCHROOT_DIR}.lock"
>  
> +BUILDCHROOT_MOUNT_CNT = "${BUILDCHROOT_DIR}.mount"
> +
>  buildchroot_do_mounts() {
>      sudo -s <<'EOSUDO'
>          ( flock 9
>          set -e
>  
>          count="1"
> -        if [ -f '${BUILDCHROOT_DIR}.mount' ]; then
> -            count=$(($(cat '${BUILDCHROOT_DIR}.mount') + 1))
> +        if [ -f "${BUILDCHROOT_MOUNT_CNT}" ]; then
> +            count=$(($(cat "${BUILDCHROOT_MOUNT_CNT}") + 1))
>          fi
> -        echo $count > '${BUILDCHROOT_DIR}.mount'
> +        echo $count > "${BUILDCHROOT_MOUNT_CNT}"
>          if [ $count -gt 1 ]; then
>              exit 0
>          fi
> @@ -77,17 +79,15 @@ buildchroot_undo_mounts() {
>          ( flock 9
>          set -e
>  
> -        if [ -f '${BUILDCHROOT_DIR}.mount' ]; then
> -            count=$(($(cat '${BUILDCHROOT_DIR}.mount') - 1))
> -            echo $count > '${BUILDCHROOT_DIR}.mount'
> -        else
> -            echo "Could not find mount counter"
> -            exit 1
> +        count="0"
> +        if [ -f "${BUILDCHROOT_MOUNT_CNT}" ]; then
> +            count=$(($(cat "${BUILDCHROOT_MOUNT_CNT}") - 1))
> +            echo $count > "${BUILDCHROOT_MOUNT_CNT}"
>          fi
>          if [ $count -gt 0 ]; then
>              exit 0
>          fi
> -        rm ${BUILDCHROOT_DIR}.mount
> +        rm -f "${BUILDCHROOT_MOUNT_CNT}"
>  
>          mountpoint -q '${BUILDCHROOT_DIR}/base-apt' && \
>              umount ${BUILDCHROOT_DIR}/base-apt && \
> diff --git a/meta/classes/rootfs.bbclass b/meta/classes/rootfs.bbclass
> index 50d6408..9ccc8e3 100644
> --- a/meta/classes/rootfs.bbclass
> +++ b/meta/classes/rootfs.bbclass
> @@ -28,6 +28,8 @@ export LC_ALL = "C"
>  
>  MOUNT_LOCKFILE = "${ROOTFSDIR}.lock"
>  
> +ROOTFS_MOUNT_CNT = "${ROOTFSDIR}.mount"
> +
>  rootfs_do_mounts[weight] = "3"
>  rootfs_do_mounts() {
>      sudo -s <<'EOSUDO'
> @@ -35,10 +37,10 @@ rootfs_do_mounts() {
>          set -e
>  
>          count="1"
> -        if [ -f '${ROOTFSDIR}.mount' ]; then
> -            count=$(($(cat '${ROOTFSDIR}.mount') + 1))
> +        if [ -f "${ROOTFS_MOUNT_CNT}" ]; then
> +            count=$(($(cat "${ROOTFS_MOUNT_CNT}") + 1))
>          fi
> -        echo $count > '${ROOTFSDIR}.mount'
> +        echo $count > "${ROOTFS_MOUNT_CNT}"
>          if [ $count -gt 1 ]; then
>              exit 0
>          fi
> @@ -79,17 +81,15 @@ rootfs_undo_mounts() {
>          ( flock 9
>          set -e
>  
> -        if [ -f '${ROOTFSDIR}.mount' ]; then
> -            count=$(($(cat '${ROOTFSDIR}.mount') - 1))
> -            echo $count > '${ROOTFSDIR}.mount'
> -        else
> -            echo "Could not find mount counter"
> -            exit 1
> +        count="0"
> +        if [ -f "${ROOTFS_MOUNT_CNT}" ]; then
> +            count=$(($(cat "${ROOTFS_MOUNT_CNT}") - 1))
> +            echo $count > "${ROOTFS_MOUNT_CNT}"
>          fi
>          if [ $count -gt 0 ]; then
>              exit 0
>          fi
> -        rm ${ROOTFSDIR}.mount
> +        rm -f "${ROOTFS_MOUNT_CNT}"
>  
>          mountpoint -q '${ROOTFSDIR}/base-apt' && \
>              umount ${ROOTFSDIR}/base-apt && \
> 

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[PATCH v1 3/5] dpkg: Remove unmount loop

[Anton Mikanovich]

Remove while loop when trying to unmount WORKDIR. Any failures of
umount should be investigated to fix the root cause.
Also make dpkg_undo_mounts do not fail if dpkg_do_mounts was not
called or it was called twice.

Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
---
 meta/classes/dpkg-base.bbclass | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg-base.bbclass
index 97661a6..6c84f3a 100644
--- a/meta/classes/dpkg-base.bbclass
+++ b/meta/classes/dpkg-base.bbclass
@@ -154,16 +154,11 @@ dpkg_do_mounts() {
 }
 
 dpkg_undo_mounts() {
-    i=1
-    while ! sudo umount ${BUILDROOT}; do
-        sleep 0.1
-        i=`expr $i + 1`
-        if [ $i -gt 100 ]; then
-            bbwarn "${BUILDROOT}: Couldn't unmount, retrying..."
-            i=1
-        fi
-    done
-    sudo rmdir ${BUILDROOT}
+    buildchroot_undo_mounts
+
+    mountpoint -q '${BUILDROOT}' && \
+        sudo umount ${BUILDROOT} && \
+        sudo rmdir ${BUILDROOT} || true
 }
 
 # Placeholder for actual dpkg_runbuild() implementation
-- 
2.25.1

Re: [PATCH v1 3/5] dpkg: Remove unmount loop

[Jan Kiszka]

On 07.07.21 18:38, Anton Mikanovich wrote:
> Remove while loop when trying to unmount WORKDIR. Any failures of
> umount should be investigated to fix the root cause.
> Also make dpkg_undo_mounts do not fail if dpkg_do_mounts was not
> called or it was called twice.
> 

This is very likely wrong: We know from the past (should be in the git
history, e.g 17d0842d) that not all subprocesses started inside the
chroot may have been terminated when we left it. That can cause
significant delays between returning from chroot and finding non-busy
mount points to complete all umounts. Now, if you silently fail umounts,
you may find inconsistently mounted chroots on the next recipe that
tries to remount them. Big fat warning sign...!

Jan

> Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
> ---
>  meta/classes/dpkg-base.bbclass | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg-base.bbclass
> index 97661a6..6c84f3a 100644
> --- a/meta/classes/dpkg-base.bbclass
> +++ b/meta/classes/dpkg-base.bbclass
> @@ -154,16 +154,11 @@ dpkg_do_mounts() {
>  }
>  
>  dpkg_undo_mounts() {
> -    i=1
> -    while ! sudo umount ${BUILDROOT}; do
> -        sleep 0.1
> -        i=`expr $i + 1`
> -        if [ $i -gt 100 ]; then
> -            bbwarn "${BUILDROOT}: Couldn't unmount, retrying..."
> -            i=1
> -        fi
> -    done
> -    sudo rmdir ${BUILDROOT}
> +    buildchroot_undo_mounts
> +
> +    mountpoint -q '${BUILDROOT}' && \
> +        sudo umount ${BUILDROOT} && \
> +        sudo rmdir ${BUILDROOT} || true
>  }
>  
>  # Placeholder for actual dpkg_runbuild() implementation
> 

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH v1 3/5] dpkg: Remove unmount loop

[Anton Mikanovich]

08.07.2021 15:21, Jan Kiszka wrote:
> This is very likely wrong: We know from the past (should be in the git
> history, e.g 17d0842d) that not all subprocesses started inside the
> chroot may have been terminated when we left it. That can cause
> significant delays between returning from chroot and finding non-busy
> mount points to complete all umounts. Now, if you silently fail umounts,
> you may find inconsistently mounted chroots on the next recipe that
> tries to remount them. Big fat warning sign...!
>
> Jan

Is that really correct to left background processes running from chroot?
Even if it is, we need to control it not to let it run forever.
Not sure how to deal with that. Set maximum run time maybe?

-- 
Anton Mikanovich
Promwad Ltd.
External service provider of ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn, Germany
+49 (89) 122 67 24-0
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov

Re: [PATCH v1 3/5] dpkg: Remove unmount loop

[Jan Kiszka]

On 08.07.21 16:35, Anton Mikanovich wrote:
> 08.07.2021 15:21, Jan Kiszka wrote:
>> This is very likely wrong: We know from the past (should be in the git
>> history, e.g 17d0842d) that not all subprocesses started inside the
>> chroot may have been terminated when we left it. That can cause
>> significant delays between returning from chroot and finding non-busy
>> mount points to complete all umounts. Now, if you silently fail umounts,
>> you may find inconsistently mounted chroots on the next recipe that
>> tries to remount them. Big fat warning sign...!
>>
>> Jan
> 
> Is that really correct to left background processes running from chroot?
> Even if it is, we need to control it not to let it run forever.
> Not sure how to deal with that. Set maximum run time maybe?
> 

See the commit I cited on how these issues were reproducible. The only
way to get rid of all pending processes - besides waiting for them - was
and likely still is putting them in containers.

If you can confirm that those issues are gone and explain, why, we can
obviously change the umounting scheme. But none of the patches so far
acknowledge these issues, thus have a high potential to kicking us back
to the point before the current lazy umounting scheme.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH v1 3/5] dpkg: Remove unmount loop

[Anton Mikanovich]

08.07.2021 15:21, Jan Kiszka wrote:
> On 07.07.21 18:38, Anton Mikanovich wrote:
>> Remove while loop when trying to unmount WORKDIR. Any failures of
>> umount should be investigated to fix the root cause.
>> Also make dpkg_undo_mounts do not fail if dpkg_do_mounts was not
>> called or it was called twice.
>>
> This is very likely wrong: We know from the past (should be in the git
> history, e.g 17d0842d) that not all subprocesses started inside the
> chroot may have been terminated when we left it. That can cause
> significant delays between returning from chroot and finding non-busy
> mount points to complete all umounts. Now, if you silently fail umounts,
> you may find inconsistently mounted chroots on the next recipe that
> tries to remount them. Big fat warning sign...!
>
> Jan

We've reproduced the issue fixed by 17d0842d on the old code (pre-17d0842d).
The issue was caused by unmounting buildchroot mounts without care of 
their usage.
The latest v5 patchset is not affected, because there is no global task 
fail handling, and per-task unmount is protected by the reference counting.

Moreover, buildchroot_undo_mounts was moved after the while loop in 
dpkg_undo_mounts in v5, so it will not fail immediately like we had it 
with '/sys/fs/cgroups is busy' failures (the real fix of 
'/sys/fs/cgroups is busy' is still in progress).

-- 
Anton Mikanovich
Promwad Ltd.
External service provider of ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn, Germany
+49 (89) 122 67 24-0
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov

[PATCH v1 4/5] image: Add reference counter

[Anton Mikanovich]

Image generation can be also made in parallel, so we need to control
the usage of image_do_mounts/image_undo_mounts.

Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
---
 meta/classes/image.bbclass | 39 ++++++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index 12d1616..b2a828c 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -52,23 +52,54 @@ DEPENDS += "${IMAGE_INSTALL}"
 ISAR_RELEASE_CMD_DEFAULT = "git -C ${LAYERDIR_core} describe --tags --dirty --match 'v[0-9].[0-9]*'"
 ISAR_RELEASE_CMD ?= "${ISAR_RELEASE_CMD_DEFAULT}"
 
+IMG_MOUNT_CNT = "${BUILDCHROOT_DIR}_image.mount"
+
 image_do_mounts() {
-    sudo flock ${MOUNT_LOCKFILE} -c ' \
+    sudo -s <<'EOSUDO'
+        ( flock 9
+        set -e
+
+        count="1"
+        if [ -f "${IMG_MOUNT_CNT}" ]; then
+            count=$(($(cat "${IMG_MOUNT_CNT}") + 1))
+        fi
+        echo $count > "${IMG_MOUNT_CNT}"
+        if [ $count -gt 1 ]; then
+            exit 0
+        fi
+
         mkdir -p "${BUILDROOT_DEPLOY}" "${BUILDROOT_ROOTFS}" "${BUILDROOT_WORK}"
         mount --bind "${DEPLOY_DIR_IMAGE}" "${BUILDROOT_DEPLOY}"
         mount --bind "${IMAGE_ROOTFS}" "${BUILDROOT_ROOTFS}"
         mount --bind "${WORKDIR}" "${BUILDROOT_WORK}"
-    '
+
+        ) 9>'${MOUNT_LOCKFILE}'
+EOSUDO
     buildchroot_do_mounts
 }
 
 image_undo_mounts() {
     buildchroot_undo_mounts
-    sudo flock ${MOUNT_LOCKFILE} -c ' \
+    sudo -s <<'EOSUDO'
+        ( flock 9
+        set -e
+
+        count="0"
+        if [ -f "${IMG_MOUNT_CNT}" ]; then
+            count=$(($(cat "${IMG_MOUNT_CNT}") - 1))
+            echo $count > "${IMG_MOUNT_CNT}"
+        fi
+        if [ $count -gt 0 ]; then
+            exit 0
+        fi
+        rm -f "${IMG_MOUNT_CNT}"
+
         umount "${BUILDROOT_DEPLOY}"
         umount "${BUILDROOT_ROOTFS}"
         umount "${BUILDROOT_WORK}"
-    '
+
+        ) 9>'${MOUNT_LOCKFILE}'
+EOSUDO
 }
 
 ROOTFSDIR = "${IMAGE_ROOTFS}"
-- 
2.25.1

[PATCH v1 5/5] events: Unmount all lost mounts at task fail

[Anton Mikanovich]

Perform mounts cleanup in case task fail.

Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
---
 meta/classes/isar-events.bbclass | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/meta/classes/isar-events.bbclass b/meta/classes/isar-events.bbclass
index 73419b4..50f5cb3 100644
--- a/meta/classes/isar-events.bbclass
+++ b/meta/classes/isar-events.bbclass
@@ -4,6 +4,25 @@
 # Copyright (C) 2015-2017 ilbers GmbH
 # Copyright (c) Siemens AG, 2018
 
+def is_mounted(d, mountpoint, subpath=None):
+    path = d.getVar(mountpoint, True)
+    if path:
+        if subpath:
+            path = os.path.join(path, subpath)
+        path = os.path.realpath(path)
+        with open('/proc/mounts') as f:
+            for line in f.readlines():
+                if path in line:
+                    return True
+    return False
+
+def safe_exec(func, d):
+    try:
+        if d.getVar(func, False):
+            bb.build.exec_func(func, d)
+    except Exception:
+        pass
+
 addhandler build_started
 
 python build_started() {
@@ -35,6 +54,14 @@ python task_failed() {
     # Avoid false positives if a second target depends on this task and retries
     # the execution after the first failure.
     os.remove(task_once_stamp(d))
+
+    # Cleanup possible lost mounts
+    if is_mounted(d, 'BUILDROOT_DEPLOY'):
+        safe_exec("image_undo_mounts", d)
+    if is_mounted(d, 'BUILDROOT'):
+        safe_exec("dpkg_undo_mounts", d)
+    if is_mounted(d, 'ROOTFSDIR', 'dev'):
+        safe_exec("rootfs_undo_mounts", d)
 }
 task_failed[eventmask] = "bb.build.TaskFailed"
 
-- 
2.25.1

Re: [PATCH v1 5/5] events: Unmount all lost mounts at task fail

[Jan Kiszka]

On 07.07.21 18:38, Anton Mikanovich wrote:
> Perform mounts cleanup in case task fail.
> 
> Signed-off-by: Anton Mikanovich <amikan@ilbers.de>
> ---
>  meta/classes/isar-events.bbclass | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/meta/classes/isar-events.bbclass b/meta/classes/isar-events.bbclass
> index 73419b4..50f5cb3 100644
> --- a/meta/classes/isar-events.bbclass
> +++ b/meta/classes/isar-events.bbclass
> @@ -4,6 +4,25 @@
>  # Copyright (C) 2015-2017 ilbers GmbH
>  # Copyright (c) Siemens AG, 2018
>  
> +def is_mounted(d, mountpoint, subpath=None):
> +    path = d.getVar(mountpoint, True)
> +    if path:
> +        if subpath:
> +            path = os.path.join(path, subpath)
> +        path = os.path.realpath(path)
> +        with open('/proc/mounts') as f:
> +            for line in f.readlines():
> +                if path in line:
> +                    return True
> +    return False
> +
> +def safe_exec(func, d):
> +    try:
> +        if d.getVar(func, False):
> +            bb.build.exec_func(func, d)
> +    except Exception:
> +        pass
> +
>  addhandler build_started
>  
>  python build_started() {
> @@ -35,6 +54,14 @@ python task_failed() {
>      # Avoid false positives if a second target depends on this task and retries
>      # the execution after the first failure.
>      os.remove(task_once_stamp(d))
> +
> +    # Cleanup possible lost mounts
> +    if is_mounted(d, 'BUILDROOT_DEPLOY'):
> +        safe_exec("image_undo_mounts", d)
> +    if is_mounted(d, 'BUILDROOT'):
> +        safe_exec("dpkg_undo_mounts", d)
> +    if is_mounted(d, 'ROOTFSDIR', 'dev'):
> +        safe_exec("rootfs_undo_mounts", d)
>  }
>  task_failed[eventmask] = "bb.build.TaskFailed"
>  
> 

No looping, so everything you miss here will continue to be cleaned up
by build_completed() - so what's the point of these changes?

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH v1 5/5] events: Unmount all lost mounts at task fail

[Anton Mikanovich]

08.07.2021 15:24, Jan Kiszka wrote:
> No looping, so everything you miss here will continue to be cleaned up
> by build_completed() - so what's the point of these changes?
>
> Jan
>
This is where WORKDIR will be unmounted to prevent double mount.
It in necessary to fix the original issue fixed by 
d21d49578e5a3b0019075d1946bd93a95914fcca and explained in previous mails.

-- 
Anton Mikanovich
Promwad Ltd.
External service provider of ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn, Germany
+49 (89) 122 67 24-0
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov

Re: [PATCH v1 5/5] events: Unmount all lost mounts at task fail

[Jan Kiszka]

On 08.07.21 16:41, Anton Mikanovich wrote:
> 08.07.2021 15:24, Jan Kiszka wrote:
>> No looping, so everything you miss here will continue to be cleaned up
>> by build_completed() - so what's the point of these changes?
>>
>> Jan
>>
> This is where WORKDIR will be unmounted to prevent double mount.
> It in necessary to fix the original issue fixed by
> d21d49578e5a3b0019075d1946bd93a95914fcca and explained in previous mails.
> 

I do get the idea of a perfect world where a single attempt to umount
works and cleanly pairs with the regular umount when the task succeeds.
The problem is that I cannot see that this has been validate to truly
catch the corner cases that the lazy, late-triggered scheme addresses so
far. If it does not handle that, these umounts are pointless. If it
does, we can drop the build_completed() handler. Having both indicates
no progress, just more complexity.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

Re: [PATCH v1 5/5] events: Unmount all lost mounts at task fail

[Baurzhan Ismagulov]

On Thu, Jul 08, 2021 at 06:28:38PM +0200, Jan Kiszka wrote:
> >> No looping, so everything you miss here will continue to be cleaned up
> >> by build_completed() - so what's the point of these changes?
...
> I do get the idea of a perfect world where a single attempt to umount
> works and cleanly pairs with the regular umount when the task succeeds.
> The problem is that I cannot see that this has been validate to truly
> catch the corner cases that the lazy, late-triggered scheme addresses so
> far. If it does not handle that, these umounts are pointless. If it
> does, we can drop the build_completed() handler. Having both indicates
> no progress, just more complexity.

Thanks Jan for the review and the use cases,

I agree that some patches could be refined and better documented.

We'll work on the topics you've raised and respond. I'm afraid we'd need your
help regarding reproducing and analysis of the old issues, because in general,
many issues so far have been rarely reproducible across different build hosts.

With kind regards,
Baurzhan.