[buildd-tools-devel] Filesystem union v3

[Jan-Marek Glogowski]

On Wed, 1 Jul 2009, Roger Leigh wrote:

> On Tue, Jun 30, 2009 at 08:05:38PM +0200, Jan-Marek Glogowski wrote:
> >
> > This version incorporates most changes from
> >
> > http://git.debian.org/?p=users/rleigh/schroot.git;a=shortlog;h=refs/heads/fs-union
> >
> > It's based on my v2 patch-set. The patches from 02 to 08 just got a little
> > cleanup based on the comments from Roger Leigh at the beginning of this month.
>
> Thanks for the new patches, but I'm afraid I just finished my work of
> merging most of the changes in your v2 patch set with my changes in the
> fs-union branch.  As a result, I'm afraid they can't be applied as-is,
> since the bulk of the changes are now present on the master branch.  I
> literally *just* finished three days of merging work when your patches
> arrived!

Seems we did the same work the last days :-( Especially splitting and
merging the "rework union" and "sync master" patch took some time.

> > > - fixed-length buffers
> > This is minimized to one occation now to read the pipe to get the filename.
>
> Is this the pipe usage in sbuild-session?

This is part of the run-parts change.

> What is the pipe used for in sbuild-session; it's not clear to me what
> the purpose is.

As all setup script are started in a sub-process, I'm using the pipe to
send the failing script name from this process, so on cleanup / stop it
just runs the executed ones.

> > > - use of dynamic cast in sbuild-session for chroot_plain.  String
> > >   comparisons are slower than dynamic cast and are *not* type-safe.
> > As chroot_directory is based on chroot_plain, dynamic cast won't work, as we
> > only want to skip the plain but not the directoty chroot.  I'm now using
> > get_run_setup_scripts() instead of the string comparison.
>
> Could you check the current master and see if this is still an issue,
> especially WRT the comments below:
>
> One problem I spotted with the code is that it breaks the chroot
> inheritance hierarchy, I think due to a misunderstanding of how
> source chroots and union chroots fit in to the overall structure.
> I fixed this up on what is now on the master branch, but I hope
> this may clear things up:
>
> * All chroot types inherit from sbuild::chroot.  Chroot types are
>   plain, directory, loopback, file, block_device and lvm_snapshot.
>
> * chroot_mountable, chroot_source and chroot_union are *not*
>   chroot types, they are *interface classes* that give the above
>   chroot types particular extra features.  They do inherit from
>   sbuild::chroot, but this is only so that they can call some
>   methods (I'll be removing this ability soon by passing in a
>   chroot reference to their methods).  This is the reason why
>   all sbuild::chroot inheritance is "virtual public".
>
> * In your patches, chroot_union was *replacing* sbuild::chroot,
>   inheritance.  If you look at my fixed version, you'll see that
>   - the classes inherit from sbuild::chroot (or other classs in
>     the chroot type hierarchy) and then use multiple inheritance
>     to additionally inherit any of the extra interfaces they
>     support
>   - in class methods, we "chain up" to the chroot type methods
>     and then any additional interface methods as appropriate.

For the original patches [02-08] that's true. All the following patches do
the same as the new patches in master and the fs-union branch. Just real
chroots inherit from sbuild::chroot.

> * I've fixed up the session flag and environment usage.
>
> * In order to support unions on block-devices, this necessitated
>   splitting block-device into a base class from which both
>   block-device and lvm-snapshot could inherit, so lvm-snapshot
>   doesn't get affected at all by union stuff.
>
> * Union filesystem support is configurable with SBUILD_FEATURE_UNION.
>
>
> So, what needs doing now:
>
> * I missed out the chroot_loopback testcase you added, so this
>   still needs adding.
>
> * I missed out UUID support and the setup script changes for that.
>   I'm not sure what the purpose of this is, and why we need it.

This isn't needed anymore. Every session now has an own underlay
directory. I used the UUID to create a common underlay directory for
loopback chroots in mounts/.

> * I also missed the run_parts changes; these can also be added.