[PATCH] isar-image-base: fix some dangling mounts

[PATCH] isar-image-base: fix some dangling mounts

[Christian Storm]

As bitbake/lib/bb/build.py's shell_trap_code() injects
`set -e` into scripts, the premature end of scripts leaves
dangling mounts around. Fix these dangling mounts by hooking
into the EXIT trap handler also setup by shell_trap_code().

Signed-off-by: Christian Storm <christian.storm@siemens.com>
---
 meta-isar/recipes-core/images/isar-image-base.bb | 10 ++++++++--
 meta/classes/dpkg-base.bbclass                   | 10 ++++++++--
 meta/classes/ext4-img.bbclass                    | 10 ++++++++--
 meta/recipes-devtools/buildchroot/buildchroot.bb | 10 ++++++++--
 4 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/meta-isar/recipes-core/images/isar-image-base.bb b/meta-isar/recipes-core/images/isar-image-base.bb
index 42aaea3..c2150b1 100644
--- a/meta-isar/recipes-core/images/isar-image-base.bb
+++ b/meta-isar/recipes-core/images/isar-image-base.bb
@@ -52,8 +52,14 @@ do_rootfs() {
         -e 's|##ISAR_DISTRO_SUITE##|${DEBDISTRONAME}|g' \
            "${WORKDIR}/multistrap.conf.in" > "${WORKDIR}/multistrap.conf"
 
-    install -d -m 555 ${IMAGE_ROOTFS}/proc
+    [ ! -d ${IMAGE_ROOTFS}/proc ] && install -d -m 555 ${IMAGE_ROOTFS}/proc
     sudo mount -t proc none ${IMAGE_ROOTFS}/proc
+    _do_rootfs_cleanup() {
+        ret=$?
+        sudo umount ${IMAGE_ROOTFS}/proc 2>/dev/null || true
+        (exit $ret) || bb_exit_handler
+    }
+    trap '_do_rootfs_cleanup' EXIT
 
     # Create root filesystem
     sudo multistrap -a ${DISTRO_ARCH} -d "${IMAGE_ROOTFS}" -f "${WORKDIR}/multistrap.conf"
@@ -62,7 +68,7 @@ do_rootfs() {
     sudo chroot ${IMAGE_ROOTFS} /${DISTRO_CONFIG_SCRIPT} ${MACHINE_SERIAL} ${BAUDRATE_TTY} \
         ${ROOTFS_DEV}
     sudo rm "${IMAGE_ROOTFS}/${DISTRO_CONFIG_SCRIPT}"
-    sudo umount ${IMAGE_ROOTFS}/proc
+    _do_rootfs_cleanup
 }
 
 addtask rootfs before do_build after do_populate
diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg-base.bbclass
index 9ded3ae..35af6d5 100644
--- a/meta/classes/dpkg-base.bbclass
+++ b/meta/classes/dpkg-base.bbclass
@@ -21,9 +21,15 @@ dpkg_runbuild() {
 do_build() {
     mkdir -p ${BUILDROOT}
     sudo mount --bind ${WORKDIR} ${BUILDROOT}
+    _do_build_cleanup() {
+        ret=$?
+        sudo umount ${BUILDROOT} 2>/dev/null || true
+        sudo rmdir ${BUILDROOT} 2>/dev/null || true
+        (exit $ret) || bb_exit_handler
+    }
+    trap '_do_build_cleanup' EXIT
     dpkg_runbuild
-    sudo umount ${BUILDROOT}
-    rm -rf ${BUILDROOT}
+    _do_build_cleanup
 }
 
 # Install package to dedicated deploy directory
diff --git a/meta/classes/ext4-img.bbclass b/meta/classes/ext4-img.bbclass
index 18a74ca..83cb137 100644
--- a/meta/classes/ext4-img.bbclass
+++ b/meta/classes/ext4-img.bbclass
@@ -19,9 +19,15 @@ do_ext4_image() {
 
     mkdir -p ${WORKDIR}/mnt
     sudo mount -o loop ${EXT4_IMAGE_FILE} ${WORKDIR}/mnt
+    _do_ext4_image_cleanup() {
+        ret=$?
+        sudo umount ${WORKDIR}/mnt 2>/dev/null || true
+        sudo rmdir ${WORKDIR}/mnt 2>/dev/null || true
+        (exit $ret) || bb_exit_handler
+    }
+    trap '_do_ext4_image_cleanup' EXIT
     sudo cp -r ${IMAGE_ROOTFS}/* ${WORKDIR}/mnt
-    sudo umount ${WORKDIR}/mnt
-    rm -r ${WORKDIR}/mnt
+    _do_ext4_image_cleanup
 }
 
 addtask ext4_image before do_build after do_copy_boot_files
diff --git a/meta/recipes-devtools/buildchroot/buildchroot.bb b/meta/recipes-devtools/buildchroot/buildchroot.bb
index ace64ba..4cae9fc 100644
--- a/meta/recipes-devtools/buildchroot/buildchroot.bb
+++ b/meta/recipes-devtools/buildchroot/buildchroot.bb
@@ -52,8 +52,14 @@ do_build() {
         -e 's|##DIR_HOOKS##|./'"$WORKDIR_REL"'/hooks_multistrap|g' \
            "${WORKDIR}/multistrap.conf.in" > "${WORKDIR}/multistrap.conf"
 
-    install -d -m 555 ${BUILDCHROOT_DIR}/proc
+    [ ! -d ${BUILDCHROOT_DIR}/proc ] && install -d -m 555 ${BUILDCHROOT_DIR}/proc
     sudo mount -t proc none ${BUILDCHROOT_DIR}/proc
+    _do_build_cleanup() {
+        ret=$?
+        sudo umount ${BUILDCHROOT_DIR}/proc 2>/dev/null || true
+        (exit $ret) || bb_exit_handler
+    }
+    trap '_do_build_cleanup' EXIT    
 
     # Create root filesystem
     sudo multistrap -a ${DISTRO_ARCH} -d "${BUILDCHROOT_DIR}" -f "${WORKDIR}/multistrap.conf"
@@ -63,5 +69,5 @@ do_build() {
 
     # Configure root filesystem
     sudo chroot ${BUILDCHROOT_DIR} /configscript.sh
-    sudo umount ${BUILDCHROOT_DIR}/proc
+    _do_build_cleanup
 }
-- 
2.15.0

Re: [PATCH] isar-image-base: fix some dangling mounts

[Alexander Smirnov]

Hi,

On 11/13/2017 03:22 PM, Christian Storm wrote:
> As bitbake/lib/bb/build.py's shell_trap_code() injects
> `set -e` into scripts, the premature end of scripts leaves
> dangling mounts around. Fix these dangling mounts by hooking
> into the EXIT trap handler also setup by shell_trap_code().

Could you please specify which mounts are dangling and how you reproduce 
this?

Alex

Re: [PATCH] isar-image-base: fix some dangling mounts

[Christian Storm]

> > As bitbake/lib/bb/build.py's shell_trap_code() injects
> > `set -e` into scripts, the premature end of scripts leaves
> > dangling mounts around. Fix these dangling mounts by hooking
> > into the EXIT trap handler also setup by shell_trap_code().
> 
> Could you please specify which mounts are dangling and how you
> reproduce this?

All those now supervised by the trap. To reproduce, e.g., for 
meta/recipes-devtools/buildchroot/buildchroot.bb, put a 
  false
after, e.g., the trap line simulating the following multistrap call to
have failed. Then, without this patch,  ${BUILDCHROOT_DIR}/proc is still
mounted. With this patch, it's unmounted.
Same procedure applies to the other cases.


Kind regards,
   Christian

-- 
Dr. Christian Storm
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Otto-Hahn-Ring 6, 81739 M�nchen, Germany

Re: [PATCH] isar-image-base: fix some dangling mounts

[Alexander Smirnov]

Hi,

On 11/13/2017 03:22 PM, Christian Storm wrote:
> As bitbake/lib/bb/build.py's shell_trap_code() injects
> `set -e` into scripts, the premature end of scripts leaves
> dangling mounts around. Fix these dangling mounts by hooking
> into the EXIT trap handler also setup by shell_trap_code().
> 

Thank you for the explanation (in another main), now I've got the idea!

> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>   meta-isar/recipes-core/images/isar-image-base.bb | 10 ++++++++--
>   meta/classes/dpkg-base.bbclass                   | 10 ++++++++--
>   meta/classes/ext4-img.bbclass                    | 10 ++++++++--
>   meta/recipes-devtools/buildchroot/buildchroot.bb | 10 ++++++++--
>   4 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/meta-isar/recipes-core/images/isar-image-base.bb b/meta-isar/recipes-core/images/isar-image-base.bb
> index 42aaea3..c2150b1 100644
> --- a/meta-isar/recipes-core/images/isar-image-base.bb
> +++ b/meta-isar/recipes-core/images/isar-image-base.bb
> @@ -52,8 +52,14 @@ do_rootfs() {
>           -e 's|##ISAR_DISTRO_SUITE##|${DEBDISTRONAME}|g' \
>              "${WORKDIR}/multistrap.conf.in" > "${WORKDIR}/multistrap.conf"
>   
> -    install -d -m 555 ${IMAGE_ROOTFS}/proc
> +    [ ! -d ${IMAGE_ROOTFS}/proc ] && install -d -m 555 ${IMAGE_ROOTFS}/proc
>       sudo mount -t proc none ${IMAGE_ROOTFS}/proc
> +    _do_rootfs_cleanup() {
> +        ret=$?
> +        sudo umount ${IMAGE_ROOTFS}/proc 2>/dev/null || true
> +        (exit $ret) || bb_exit_handler
> +    }
> +    trap '_do_rootfs_cleanup' EXIT
>   
>       # Create root filesystem
>       sudo multistrap -a ${DISTRO_ARCH} -d "${IMAGE_ROOTFS}" -f "${WORKDIR}/multistrap.conf"
> @@ -62,7 +68,7 @@ do_rootfs() {
>       sudo chroot ${IMAGE_ROOTFS} /${DISTRO_CONFIG_SCRIPT} ${MACHINE_SERIAL} ${BAUDRATE_TTY} \
>           ${ROOTFS_DEV}
>       sudo rm "${IMAGE_ROOTFS}/${DISTRO_CONFIG_SCRIPT}"
> -    sudo umount ${IMAGE_ROOTFS}/proc
> +    _do_rootfs_cleanup
>   }
>   
>   addtask rootfs before do_build after do_populate
> diff --git a/meta/classes/dpkg-base.bbclass b/meta/classes/dpkg-base.bbclass
> index 9ded3ae..35af6d5 100644
> --- a/meta/classes/dpkg-base.bbclass
> +++ b/meta/classes/dpkg-base.bbclass
> @@ -21,9 +21,15 @@ dpkg_runbuild() {
>   do_build() {
>       mkdir -p ${BUILDROOT}
>       sudo mount --bind ${WORKDIR} ${BUILDROOT}
> +    _do_build_cleanup() {
> +        ret=$?
> +        sudo umount ${BUILDROOT} 2>/dev/null || true
> +        sudo rmdir ${BUILDROOT} 2>/dev/null || true
> +        (exit $ret) || bb_exit_handler
> +    }
> +    trap '_do_build_cleanup' EXIT
>       dpkg_runbuild
> -    sudo umount ${BUILDROOT}
> -    rm -rf ${BUILDROOT}
> +    _do_build_cleanup
>   }
>   
>   # Install package to dedicated deploy directory
> diff --git a/meta/classes/ext4-img.bbclass b/meta/classes/ext4-img.bbclass
> index 18a74ca..83cb137 100644
> --- a/meta/classes/ext4-img.bbclass
> +++ b/meta/classes/ext4-img.bbclass
> @@ -19,9 +19,15 @@ do_ext4_image() {
>   
>       mkdir -p ${WORKDIR}/mnt
>       sudo mount -o loop ${EXT4_IMAGE_FILE} ${WORKDIR}/mnt
> +    _do_ext4_image_cleanup() {
> +        ret=$?
> +        sudo umount ${WORKDIR}/mnt 2>/dev/null || true
> +        sudo rmdir ${WORKDIR}/mnt 2>/dev/null || true
> +        (exit $ret) || bb_exit_handler
> +    }
> +    trap '_do_ext4_image_cleanup' EXIT
>       sudo cp -r ${IMAGE_ROOTFS}/* ${WORKDIR}/mnt
> -    sudo umount ${WORKDIR}/mnt
> -    rm -r ${WORKDIR}/mnt
> +    _do_ext4_image_cleanup
>   }
>   
>   addtask ext4_image before do_build after do_copy_boot_files
> diff --git a/meta/recipes-devtools/buildchroot/buildchroot.bb b/meta/recipes-devtools/buildchroot/buildchroot.bb
> index ace64ba..4cae9fc 100644
> --- a/meta/recipes-devtools/buildchroot/buildchroot.bb
> +++ b/meta/recipes-devtools/buildchroot/buildchroot.bb
> @@ -52,8 +52,14 @@ do_build() {
>           -e 's|##DIR_HOOKS##|./'"$WORKDIR_REL"'/hooks_multistrap|g' \
>              "${WORKDIR}/multistrap.conf.in" > "${WORKDIR}/multistrap.conf"
>   
> -    install -d -m 555 ${BUILDCHROOT_DIR}/proc
> +    [ ! -d ${BUILDCHROOT_DIR}/proc ] && install -d -m 555 ${BUILDCHROOT_DIR}/proc
>       sudo mount -t proc none ${BUILDCHROOT_DIR}/proc
> +    _do_build_cleanup() {
> +        ret=$?
> +        sudo umount ${BUILDCHROOT_DIR}/proc 2>/dev/null || true
> +        (exit $ret) || bb_exit_handler
> +    }
> +    trap '_do_build_cleanup' EXIT
>   
>       # Create root filesystem
>       sudo multistrap -a ${DISTRO_ARCH} -d "${BUILDCHROOT_DIR}" -f "${WORKDIR}/multistrap.conf"
> @@ -63,5 +69,5 @@ do_build() {
>   
>       # Configure root filesystem
>       sudo chroot ${BUILDCHROOT_DIR} /configscript.sh
> -    sudo umount ${BUILDCHROOT_DIR}/proc
> +    _do_build_cleanup
>   }
> 

Here I see that you insert quite similar patterns across the Isar code, 
would it be better to have one global clean up function like?

do_isar_mount_cleanup () {
     if [ -d "${WORKDIR}/mnt" ]; then
         # umount and remove
     fi

     if [ -d "${WORKDIR}/rootfs/proc" ]; then
         # umount and remove
     fi

     if [ -d "${BUILDCHROOT_DIR}/proc" ]; then
         # umount and remove
     fi

     etc...
}

This approach will significantly simplify maintenance of clean up traps 
and adding of new possible mounts to Isar. What do you think?

Alex

Re: [PATCH] isar-image-base: fix some dangling mounts

[Henning Schild]

Cool, i suggested before to use exception handling for that problem,
just did not know how that is done in bb.

Henning

Am Mon, 13 Nov 2017 13:22:39 +0100
schrieb "[ext] Christian Storm" <christian.storm@siemens.com>:

> As bitbake/lib/bb/build.py's shell_trap_code() injects
> `set -e` into scripts, the premature end of scripts leaves
> dangling mounts around. Fix these dangling mounts by hooking
> into the EXIT trap handler also setup by shell_trap_code().
> 
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>  meta-isar/recipes-core/images/isar-image-base.bb | 10 ++++++++--
>  meta/classes/dpkg-base.bbclass                   | 10 ++++++++--
>  meta/classes/ext4-img.bbclass                    | 10 ++++++++--
>  meta/recipes-devtools/buildchroot/buildchroot.bb | 10 ++++++++--
>  4 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/meta-isar/recipes-core/images/isar-image-base.bb
> b/meta-isar/recipes-core/images/isar-image-base.bb index
> 42aaea3..c2150b1 100644 ---
> a/meta-isar/recipes-core/images/isar-image-base.bb +++
> b/meta-isar/recipes-core/images/isar-image-base.bb @@ -52,8 +52,14 @@
> do_rootfs() { -e 's|##ISAR_DISTRO_SUITE##|${DEBDISTRONAME}|g' \
>             "${WORKDIR}/multistrap.conf.in" >
> "${WORKDIR}/multistrap.conf" 
> -    install -d -m 555 ${IMAGE_ROOTFS}/proc
> +    [ ! -d ${IMAGE_ROOTFS}/proc ] && install -d -m 555
> ${IMAGE_ROOTFS}/proc sudo mount -t proc none ${IMAGE_ROOTFS}/proc
> +    _do_rootfs_cleanup() {
> +        ret=$?
> +        sudo umount ${IMAGE_ROOTFS}/proc 2>/dev/null || true
> +        (exit $ret) || bb_exit_handler
> +    }
> +    trap '_do_rootfs_cleanup' EXIT
>  
>      # Create root filesystem
>      sudo multistrap -a ${DISTRO_ARCH} -d "${IMAGE_ROOTFS}" -f
> "${WORKDIR}/multistrap.conf" @@ -62,7 +68,7 @@ do_rootfs() {
>      sudo chroot ${IMAGE_ROOTFS} /${DISTRO_CONFIG_SCRIPT}
> ${MACHINE_SERIAL} ${BAUDRATE_TTY} \ ${ROOTFS_DEV}
>      sudo rm "${IMAGE_ROOTFS}/${DISTRO_CONFIG_SCRIPT}"
> -    sudo umount ${IMAGE_ROOTFS}/proc
> +    _do_rootfs_cleanup
>  }
>  
>  addtask rootfs before do_build after do_populate
> diff --git a/meta/classes/dpkg-base.bbclass
> b/meta/classes/dpkg-base.bbclass index 9ded3ae..35af6d5 100644
> --- a/meta/classes/dpkg-base.bbclass
> +++ b/meta/classes/dpkg-base.bbclass
> @@ -21,9 +21,15 @@ dpkg_runbuild() {
>  do_build() {
>      mkdir -p ${BUILDROOT}
>      sudo mount --bind ${WORKDIR} ${BUILDROOT}
> +    _do_build_cleanup() {
> +        ret=$?
> +        sudo umount ${BUILDROOT} 2>/dev/null || true
> +        sudo rmdir ${BUILDROOT} 2>/dev/null || true
> +        (exit $ret) || bb_exit_handler
> +    }
> +    trap '_do_build_cleanup' EXIT
>      dpkg_runbuild
> -    sudo umount ${BUILDROOT}
> -    rm -rf ${BUILDROOT}
> +    _do_build_cleanup
>  }
>  
>  # Install package to dedicated deploy directory
> diff --git a/meta/classes/ext4-img.bbclass
> b/meta/classes/ext4-img.bbclass index 18a74ca..83cb137 100644
> --- a/meta/classes/ext4-img.bbclass
> +++ b/meta/classes/ext4-img.bbclass
> @@ -19,9 +19,15 @@ do_ext4_image() {
>  
>      mkdir -p ${WORKDIR}/mnt
>      sudo mount -o loop ${EXT4_IMAGE_FILE} ${WORKDIR}/mnt
> +    _do_ext4_image_cleanup() {
> +        ret=$?
> +        sudo umount ${WORKDIR}/mnt 2>/dev/null || true
> +        sudo rmdir ${WORKDIR}/mnt 2>/dev/null || true
> +        (exit $ret) || bb_exit_handler
> +    }
> +    trap '_do_ext4_image_cleanup' EXIT
>      sudo cp -r ${IMAGE_ROOTFS}/* ${WORKDIR}/mnt
> -    sudo umount ${WORKDIR}/mnt
> -    rm -r ${WORKDIR}/mnt
> +    _do_ext4_image_cleanup
>  }
>  
>  addtask ext4_image before do_build after do_copy_boot_files
> diff --git a/meta/recipes-devtools/buildchroot/buildchroot.bb
> b/meta/recipes-devtools/buildchroot/buildchroot.bb index
> ace64ba..4cae9fc 100644 ---
> a/meta/recipes-devtools/buildchroot/buildchroot.bb +++
> b/meta/recipes-devtools/buildchroot/buildchroot.bb @@ -52,8 +52,14 @@
> do_build() { -e
> 's|##DIR_HOOKS##|./'"$WORKDIR_REL"'/hooks_multistrap|g' \
> "${WORKDIR}/multistrap.conf.in" > "${WORKDIR}/multistrap.conf" 
> -    install -d -m 555 ${BUILDCHROOT_DIR}/proc
> +    [ ! -d ${BUILDCHROOT_DIR}/proc ] && install -d -m 555
> ${BUILDCHROOT_DIR}/proc sudo mount -t proc none
> ${BUILDCHROOT_DIR}/proc
> +    _do_build_cleanup() {
> +        ret=$?
> +        sudo umount ${BUILDCHROOT_DIR}/proc 2>/dev/null || true
> +        (exit $ret) || bb_exit_handler
> +    }
> +    trap '_do_build_cleanup' EXIT    
>  
>      # Create root filesystem
>      sudo multistrap -a ${DISTRO_ARCH} -d "${BUILDCHROOT_DIR}" -f
> "${WORKDIR}/multistrap.conf" @@ -63,5 +69,5 @@ do_build() {
>  
>      # Configure root filesystem
>      sudo chroot ${BUILDCHROOT_DIR} /configscript.sh
> -    sudo umount ${BUILDCHROOT_DIR}/proc
> +    _do_build_cleanup
>  }

Re: [PATCH] isar-image-base: fix some dangling mounts

[Christian Storm]

> [...]
> Here I see that you insert quite similar patterns across the Isar code, 
> would it be better to have one global clean up function like?
> 
> do_isar_mount_cleanup () {
>      if [ -d "${WORKDIR}/mnt" ]; then
>          # umount and remove
>      fi
> 
>      if [ -d "${WORKDIR}/rootfs/proc" ]; then
>          # umount and remove
>      fi
> 
>      if [ -d "${BUILDCHROOT_DIR}/proc" ]; then
>          # umount and remove
>      fi
> 
>      etc...
> }
> 
> This approach will significantly simplify maintenance of clean up traps 
> and adding of new possible mounts to Isar. What do you think?

I'd say it depends :)
It breaks if you rely on keeping something mounted while executing a
child script that does unmount everything, following your proposal. If
we can make sure that such a situation never occurs, we may consolidate
this. For now, it's rather local just undoing the mounts done in the
scope of the script.
So, in the end, I think it's a matter of taste and contracts between the
different parts of Isar.

What one may consider to "de-duplicate" is a stack of mounted directories
being pushed to and unmounted on script exit. So, something like
  push_umount ${IMAGE_ROOTFS}/proc
and then the EXIT trap enumerates all those entries, unmounting them.
The same has to be done for removing the directories, if applicable.



Besten Gru�,
   Christian

-- 
Dr. Christian Storm
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Otto-Hahn-Ring 6, 81739 M�nchen, Germany

Re: [PATCH] isar-image-base: fix some dangling mounts

[Alexander Smirnov]

On 11/13/2017 05:24 PM, Christian Storm wrote:
>> [...]
>> Here I see that you insert quite similar patterns across the Isar code,
>> would it be better to have one global clean up function like?
>>
>> do_isar_mount_cleanup () {
>>       if [ -d "${WORKDIR}/mnt" ]; then
>>           # umount and remove
>>       fi
>>
>>       if [ -d "${WORKDIR}/rootfs/proc" ]; then
>>           # umount and remove
>>       fi
>>
>>       if [ -d "${BUILDCHROOT_DIR}/proc" ]; then
>>           # umount and remove
>>       fi
>>
>>       etc...
>> }
>>
>> This approach will significantly simplify maintenance of clean up traps
>> and adding of new possible mounts to Isar. What do you think?
> 
> I'd say it depends :)
> It breaks if you rely on keeping something mounted while executing a
> child script that does unmount everything, following your proposal. If
> we can make sure that such a situation never occurs, we may consolidate
> this. For now, it's rather local just undoing the mounts done in the
> scope of the script.
> So, in the end, I think it's a matter of taste and contracts between the
> different parts of Isar.
> 

Ok.

> What one may consider to "de-duplicate" is a stack of mounted directories
> being pushed to and unmounted on script exit. So, something like
>    push_umount ${IMAGE_ROOTFS}/proc
> and then the EXIT trap enumerates all those entries, unmounting them.
> The same has to be done for removing the directories, if applicable.

This sounds reasonable, will try to implement this?

BTW: for me it's not the point for me to block current patch, it'd be 
very helpful to have it in the tree now.

Alex

Re: [PATCH] isar-image-base: fix some dangling mounts

[Christian Storm]

> >> [...]
> >> Here I see that you insert quite similar patterns across the Isar code,
> >> would it be better to have one global clean up function like?
> >>
> >> do_isar_mount_cleanup () {
> >>       if [ -d "${WORKDIR}/mnt" ]; then
> >>           # umount and remove
> >>       fi
> >>
> >>       if [ -d "${WORKDIR}/rootfs/proc" ]; then
> >>           # umount and remove
> >>       fi
> >>
> >>       if [ -d "${BUILDCHROOT_DIR}/proc" ]; then
> >>           # umount and remove
> >>       fi
> >>
> >>       etc...
> >> }
> >>
> >> This approach will significantly simplify maintenance of clean up traps
> >> and adding of new possible mounts to Isar. What do you think?
> > 
> > I'd say it depends :)
> > It breaks if you rely on keeping something mounted while executing a
> > child script that does unmount everything, following your proposal. If
> > we can make sure that such a situation never occurs, we may consolidate
> > this. For now, it's rather local just undoing the mounts done in the
> > scope of the script.
> > So, in the end, I think it's a matter of taste and contracts between the
> > different parts of Isar.
> > 
> 
> Ok.
> 
> > What one may consider to "de-duplicate" is a stack of mounted directories
> > being pushed to and unmounted on script exit. So, something like
> >    push_umount ${IMAGE_ROOTFS}/proc
> > and then the EXIT trap enumerates all those entries, unmounting them.
> > The same has to be done for removing the directories, if applicable.
> 
> This sounds reasonable, will try to implement this?
> 
> BTW: for me it's not the point for me to block current patch, it'd be 
> very helpful to have it in the tree now.

OK, then let's merge it and create a github issue to refactor it later
as I do have more mount-related stuff in my queue :)
Once all mount-related patches are merged, we do have a clearer picture
and can account for all this at once in terms of a discussion and a
proper concept on the github issue. So, I propose we merge this and I'll
send the other patches out once ready and then we do a refactoring with
discussion on the right way to do it on the github issue. My proposal
with the stack may serve as a starting point for discussion on the
github issue.

What do you think? Would that be OK for you?



Besten Gru�,
   Christian

-- 
Dr. Christian Storm
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Otto-Hahn-Ring 6, 81739 M�nchen, Germany

Re: [PATCH] isar-image-base: fix some dangling mounts

[Alexander Smirnov]

On 11/14/2017 10:36 AM, Christian Storm wrote:
> 
>>>> [...]
>>>> Here I see that you insert quite similar patterns across the Isar code,
>>>> would it be better to have one global clean up function like?
>>>>
>>>> do_isar_mount_cleanup () {
>>>>        if [ -d "${WORKDIR}/mnt" ]; then
>>>>            # umount and remove
>>>>        fi
>>>>
>>>>        if [ -d "${WORKDIR}/rootfs/proc" ]; then
>>>>            # umount and remove
>>>>        fi
>>>>
>>>>        if [ -d "${BUILDCHROOT_DIR}/proc" ]; then
>>>>            # umount and remove
>>>>        fi
>>>>
>>>>        etc...
>>>> }
>>>>
>>>> This approach will significantly simplify maintenance of clean up traps
>>>> and adding of new possible mounts to Isar. What do you think?
>>>
>>> I'd say it depends :)
>>> It breaks if you rely on keeping something mounted while executing a
>>> child script that does unmount everything, following your proposal. If
>>> we can make sure that such a situation never occurs, we may consolidate
>>> this. For now, it's rather local just undoing the mounts done in the
>>> scope of the script.
>>> So, in the end, I think it's a matter of taste and contracts between the
>>> different parts of Isar.
>>>
>>
>> Ok.
>>
>>> What one may consider to "de-duplicate" is a stack of mounted directories
>>> being pushed to and unmounted on script exit. So, something like
>>>     push_umount ${IMAGE_ROOTFS}/proc
>>> and then the EXIT trap enumerates all those entries, unmounting them.
>>> The same has to be done for removing the directories, if applicable.
>>
>> This sounds reasonable, will try to implement this?
>>
>> BTW: for me it's not the point for me to block current patch, it'd be
>> very helpful to have it in the tree now.
> 
> OK, then let's merge it and create a github issue to refactor it later
> as I do have more mount-related stuff in my queue :)
> Once all mount-related patches are merged, we do have a clearer picture
> and can account for all this at once in terms of a discussion and a
> proper concept on the github issue. So, I propose we merge this and I'll
> send the other patches out once ready and then we do a refactoring with
> discussion on the right way to do it on the github issue. My proposal
> with the stack may serve as a starting point for discussion on the
> github issue.
> 
> What do you think? Would that be OK for you?

Sure! Please create then an issue.

Alex

Re: [PATCH] isar-image-base: fix some dangling mounts

[Christian Storm]

> >>>> [...]
> >>>> Here I see that you insert quite similar patterns across the Isar code,
> >>>> would it be better to have one global clean up function like?
> >>>>
> >>>> do_isar_mount_cleanup () {
> >>>>        if [ -d "${WORKDIR}/mnt" ]; then
> >>>>            # umount and remove
> >>>>        fi
> >>>>
> >>>>        if [ -d "${WORKDIR}/rootfs/proc" ]; then
> >>>>            # umount and remove
> >>>>        fi
> >>>>
> >>>>        if [ -d "${BUILDCHROOT_DIR}/proc" ]; then
> >>>>            # umount and remove
> >>>>        fi
> >>>>
> >>>>        etc...
> >>>> }
> >>>>
> >>>> This approach will significantly simplify maintenance of clean up traps
> >>>> and adding of new possible mounts to Isar. What do you think?
> >>>
> >>> I'd say it depends :)
> >>> It breaks if you rely on keeping something mounted while executing a
> >>> child script that does unmount everything, following your proposal. If
> >>> we can make sure that such a situation never occurs, we may consolidate
> >>> this. For now, it's rather local just undoing the mounts done in the
> >>> scope of the script.
> >>> So, in the end, I think it's a matter of taste and contracts between the
> >>> different parts of Isar.
> >>>
> >>
> >> Ok.
> >>
> >>> What one may consider to "de-duplicate" is a stack of mounted directories
> >>> being pushed to and unmounted on script exit. So, something like
> >>>     push_umount ${IMAGE_ROOTFS}/proc
> >>> and then the EXIT trap enumerates all those entries, unmounting them.
> >>> The same has to be done for removing the directories, if applicable.
> >>
> >> This sounds reasonable, will try to implement this?
> >>
> >> BTW: for me it's not the point for me to block current patch, it'd be
> >> very helpful to have it in the tree now.
> > 
> > OK, then let's merge it and create a github issue to refactor it later
> > as I do have more mount-related stuff in my queue :)
> > Once all mount-related patches are merged, we do have a clearer picture
> > and can account for all this at once in terms of a discussion and a
> > proper concept on the github issue. So, I propose we merge this and I'll
> > send the other patches out once ready and then we do a refactoring with
> > discussion on the right way to do it on the github issue. My proposal
> > with the stack may serve as a starting point for discussion on the
> > github issue.
> > 
> > What do you think? Would that be OK for you?
> 
> Sure! Please create then an issue.


OK, here's the issue: https://github.com/ilbers/isar/issues/38


Besten Gru�,
   Christian

-- 
Dr. Christian Storm
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Otto-Hahn-Ring 6, 81739 M�nchen, Germany

Re: [PATCH] isar-image-base: fix some dangling mounts

[Alexander Smirnov]

On 11/13/2017 03:22 PM, Christian Storm wrote:
> As bitbake/lib/bb/build.py's shell_trap_code() injects
> `set -e` into scripts, the premature end of scripts leaves
> dangling mounts around. Fix these dangling mounts by hooking
> into the EXIT trap handler also setup by shell_trap_code().
> 
> Signed-off-by: Christian Storm <christian.storm@siemens.com>

Fixed trailing whitespaces, applied to next. Thanks!

Alex