[buildd-tools-devel] Filesystem union v3

[Roger Leigh]

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!

> > - 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?

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

> > - 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.

* 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.

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

* The patches you just posted very likely contain bits I didn't
  merge due to either just missing them by accident or
  deliberately choosing not to merge them (such as the UUID
  stuff).  It would be great if you could go through and see
  if we are missing anything that still needs adding.

* Testing.  Lots and lots of testing.  I've done all the merging
  work, but not yet seen what's broken.  We need to go through
  and make sure all the different chroot types still function
  correctly and fix and add testcases for all the breakage.
  Previously, I was seeing breakage with --begin-session where
  --run-session would break due to some of the session data
  being missing; I'll need to check that again.

* Documentation.  I think that sbuild.conf.5.in is ok, but
  sbuild-setup.5.in needs the UNION_* options documenting.


Many thanks for all your work so far; it looks like it's now
coming together nicely.  Once the final bits are tidied up it
should be great.


Regards,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20090701/dab97476/attachment.pgp>