Bug#789694: schroot: Please consider patch to fix it on hurd-i386

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug#789694: schroot: Please consider patch to fix it on hurd-i386

Gabriele Giacone-3
Package: schroot
Version: 1.6.10-1+b1
Severity: wishlist
User: [hidden email]
Usertags: hurd

Dear maintainer,

attached patch makes schroot working on hurd, has been being tested on
exodar porterbox since many months.
It works around #763932 bug.

More info in threads at
 https://lists.debian.org/debian-hurd/2014/09/msg00058.html
 https://lists.debian.org/debian-hurd/2015/05/msg00063.html

Thanks for considering.

--
G..e
Reply | Threaded
Open this post in threaded view
|

Bug#789694: schroot: Please consider patch to fix it on hurd-i386

Samuel Thibault-8
Gabriele Giacone, le Tue 23 Jun 2015 15:42:52 +0200, a écrit :
> More info in threads at
>  https://lists.debian.org/debian-hurd/2014/09/msg00058.html
>  https://lists.debian.org/debian-hurd/2015/05/msg00063.html

We also need a summary, because there is a lot of information spread
across the whole thread, it's hard to determine how it fits together.

AIUI, what we have is:

- debootstrap sets up a passive (i.e. permanent) bind mount from /proc
to $CHROOT/proc
- schroot sets up other active bind mounts (e.g. for /dev, /servers,
etc.) as added in gnu/fstab from your patch
- do_umount_all "$CHROOT_MOUNT_LOCATION" does not work on the Hurd
because of bug #763932, that's why the second do_umount_all call.
- that second call (do_umount_all "${CHROOT_FILE_UNPACK_DIR}/${SESSION_ID}")
manages to umount /dev, /servers etc. but not /proc because that one is
passive, and umount doesn't unmount passive mounts. This is why the
addition of the explicit settrans to make it go away.

Please correct anything wrong in this summary.  Notably I don't think
debootstrap uses a passive translator, and thus I don't see why the
second do_umount_all shouldn't be able to umount $CHROOT/proc actually.

That was the kind of summary you should have attached with your patch.
Please really take the habit of writing this kind of summary, otherwise
it'd take a huge lot of time for the maintainer to gather, thus
considerably lowering the probability that he works on the bug at all.

> diff --git a/etc/setup.d/10mount b/etc/setup.d/10mount
> index 27c18d0..884c36a 100755
> --- a/etc/setup.d/10mount
> +++ b/etc/setup.d/10mount
> @@ -224,6 +224,21 @@ if [ "$CHROOT_TYPE" = "directory" ] \
>      elif [ $STAGE = "setup-stop" ]; then
>  
>          do_umount_all "$CHROOT_MOUNT_LOCATION"
> +        #
> +        # If CHROOT_TYPE is "file", _UNPACK_DIR is bindmounted on
> +        # _MOUNT_LOCATION. Hurd does need to also bindmount /dev and
> +        # /servers on respectively _MOUNT_LOCATION/dev and
> +        # _MOUNT_LOCATION/servers, but due to #763932, currently
> +        # bindmounts-on-bindmounts result mounted on first bindmount
> +        # source device i.e. _UNPACK_DIR.
> +        # Workaround also umounts filesystems under _UNPACK_DIR.
> +        #
> +        if [ "$CHROOT_TYPE" = "file" -a "$(uname -s)" = "GNU" ]; then
> +            do_umount_all "${CHROOT_FILE_UNPACK_DIR}/${SESSION_ID}"
> +            info "Unmounting ${CHROOT_FILE_UNPACK_DIR}/${SESSION_ID}/proc"
> +            settrans -apg "${CHROOT_FILE_UNPACK_DIR}/${SESSION_ID}/proc" || exit 1
> +        fi
> +
>          if [ "$CREATE_UNION" = "yes" ]; then
>              do_umount_all "$CHROOT_UNION_UNDERLAY_DIRECTORY"
>          fi


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Bug#789694: schroot: Please consider patch to fix it on hurd-i386

Samuel Thibault-8
Samuel Thibault, le Thu 09 Jul 2015 10:45:04 +0200, a écrit :
>  Notably I don't think debootstrap uses a passive translator, and thus
> I don't see why the second do_umount_all shouldn't be able to umount
> $CHROOT/proc actually.

I have thus commited everything but that explicit settrans, which I
don't understand why it would be needed.

Samuel


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Bug#789694: schroot: Please consider patch to fix it on hurd-i386

Gabriele Giacone-3
In reply to this post by Samuel Thibault-8
On Thu, Jul 9, 2015 at 10:45 AM, Samuel Thibault <[hidden email]> wrote:
> - debootstrap sets up a passive (i.e. permanent) bind mount from /proc
> to $CHROOT/proc

debootstrap in hurd case just runs "setup-translators -K", which sets
another passive /proc under chroot, and it bindmounts /dev and
/servers.
Bindmounts are active.
BTW patch fixes CHROOT_TYPE=file case only, I didn't spend time on
other cases. So how one creates the chroot tarball should not actually
matter.

> - schroot sets up other active bind mounts (e.g. for /dev, /servers,
> etc.) as added in gnu/fstab from your patch

Yes, /proc /dev /servers in gnu/fstab.

> - do_umount_all "$CHROOT_MOUNT_LOCATION" does not work on the Hurd
> because of bug #763932, that's why the second do_umount_all call.

Correct.

> - that second call (do_umount_all "${CHROOT_FILE_UNPACK_DIR}/${SESSION_ID}")
> manages to umount /dev, /servers etc. but not /proc because that one is
> passive, and umount doesn't unmount passive mounts. This is why the
> addition of the explicit settrans to make it go away.

Correct.

> Please correct anything wrong in this summary.  Notably I don't think
> debootstrap uses a passive translator, and thus I don't see why the
> second do_umount_all shouldn't be able to umount $CHROOT/proc actually.

As said above, debootstrap does create /proc passive translator under
chroot and maybe that would break chroot removal in CHROOT_TYPE!=file
cases. Not tested.
In CHROOT_TYPE=file case, tarball gets uncompressed, no passive
translators around, /proc /dev /servers get bindmounted and everything
works and gets removed fine. Unless... one runs setup-translators by
hand or by upgrading hurd package in chroot, that will set /proc
passive translator, which will make chroot removal fail.

I already tried to explain it at [0][1], probably fixable by making
setup-translators not to set passive /proc under chroots if already
bindmounted, as said [2] no idea how to detect that.

[0] https://lists.debian.org/debian-hurd/2014/09/msg00058.html
[1] https://lists.debian.org/debian-hurd/2015/05/msg00069.html
[2] https://lists.debian.org/debian-hurd/2014/10/msg00007.html

--
G..e


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Bug#789694: [buildd-tools-devel] Bug#789694: schroot: Please consider patch to fix it on hurd-i386

Roger Leigh-3
In reply to this post by Gabriele Giacone-3
On 23/06/2015 13:42, Gabriele Giacone wrote:

>
> attached patch makes schroot working on hurd, has been being tested on
> exodar porterbox since many months.
> It works around #763932 bug.

Dear Gabriele and Samuel,

I just want to check on the state of this patch and add a few notes.

I'm not familiar with the hurd details, so really just wanted to check
if any changes were required before it was merged.  Note that I've
opened pull requests here:

   https://github.com/codelibre-net/schroot/pull/7 (master)
   https://github.com/codelibre-net/schroot/pull/6 (1.6 branch)

Please feel free to add any comments there if needed.

Regarding Samuel's commit to the alioth master branch: this is no longer
the canonical upstream repo for schroot, so committing here is not a
great idea; it's now basically a mirror of the actual github repo, so
and commits are liable to be lost.  The above two pull requests are your
commit split into two and rebased onto the 1.6 branch (which is what's
currently in Debian unstable).

If you're happy with both of the above pull requests, I'll merge them
and make a new 1.6 release.


Kind regards,
Roger


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Bug#789694: [buildd-tools-devel] Bug#789694: schroot: Please consider patch to fix it on hurd-i386

Samuel Thibault-8
Roger Leigh, le Sun 26 Jul 2015 16:26:22 +0000, a écrit :
> Regarding Samuel's commit to the alioth master branch: this is no longer the
> canonical upstream repo for schroot,

Aow, ok.

> If you're happy with both of the above pull requests, I'll merge them and
> make a new 1.6 release.

Yes, please.

Samuel


--
To UNSUBSCRIBE, email to [hidden email]
with a subject of "unsubscribe". Trouble? Contact [hidden email]

Reply | Threaded
Open this post in threaded view
|

Bug#789694: [buildd-tools-devel] Bug#789694: schroot: Please consider patch to fix it on hurd-i386

Samuel Thibault-8
Hello,

Samuel Thibault, on Thu 30 Jul 2015 20:34:49 +0200, wrote:

> Roger Leigh, le Sun 26 Jul 2015 16:26:22 +0000, a écrit :
> > Regarding Samuel's commit to the alioth master branch: this is no longer the
> > canonical upstream repo for schroot,
>
> Aow, ok.
>
> > If you're happy with both of the above pull requests, I'll merge them and
> > make a new 1.6 release.
>
> Yes, please.

Are there any news about this?

Thanks,
Samuel

Reply | Threaded
Open this post in threaded view
|

Bug#789694: [buildd-tools-devel] Bug#789694: schroot: Please consider patch to fix it on hurd-i386

Roger Leigh-3
On 15/02/2016 22:31, Samuel Thibault wrote:

> Hello,
>
> Samuel Thibault, on Thu 30 Jul 2015 20:34:49 +0200, wrote:
>> Roger Leigh, le Sun 26 Jul 2015 16:26:22 +0000, a écrit :
>>> Regarding Samuel's commit to the alioth master branch: this is no longer the
>>> canonical upstream repo for schroot,
>>
>> Aow, ok.
>>
>>> If you're happy with both of the above pull requests, I'll merge them and
>>> make a new 1.6 release.
>>
>> Yes, please.
>
> Are there any news about this?

Hi,

These were merged into the 1.6 and master branches, but I've yet to make
a new release.  There are some other pending PRs to finish review and
test, and I can make a release after that now that I have some Debian
VMs to build and test in.


Regards,
Roger