[buildd-tools-devel] Filesystem union v2

[Roger Leigh]

On Wed, May 20, 2009 at 07:55:50PM +0200, Jan-Marek Glogowski wrote:
> This is the stripped down version of the filesystem union patchset I have
> sent about two months ago.
> 
>    8.1% etc/setup.d/
>   62.9% sbuild/
>   25.8% test/
> 
>  39 files changed, 1498 insertions(+), 228 deletions(-)
> 
> It includes patches for all chroot types, but I just tested directory and
> loopback chroots.
> 
> Thanks for review and testing.

Thanks.  I just got back from holiday and took a quick look through.
I'm about to go to a conference in Finland, so won't be able to do
anything with it until after next weekend.  I'll give it a much more
thorough review then.

- It applies cleanly against current git master, which is great.
- I like the change to deriving directory from plain.  However, it uses
  "virtual public" when not needed.  Only "chroot" should be virtual
  public, since it's the common base class when using multiple
  inheritance.  Nothing else should need it.  Ideally I want all
  extensions such as source, fs_union to be pure interface classes
  with no direct inheritance of chroot.  But we will fix this up later.
  You might need to make chroot_source virtual public when adding
  support for lvm_snapshot since it's provided by lvm_snapshot *and*
  chroot_fs_union; this is one reason I want a true interface class.
- However, there are a number of things I noticed, such as:
  + there's a number of places where you are using
    - fixed-length buffers
    - passing pointers to objects rather than references; use
      references or const references only.  Unless you're interfacing
      with C, there's no need to *ever* *ever* use an unsafe bare
      pointer.  If you must use a pointer, use a *smartpointer* like
      std::tr1::shared_ptr.
    - using fixed-length buffers for POSIX functions like getcwd.
      This will break on GNU/Hurd.  We already use Boost::Filesystem to
      do this correctly in other places IIRC.
  + 'chroot_fs_union *overlay = 0;' Why assign to NULL when you can
    assign the dynamic cast directly?  Use RAII idioms wherever possible
    (Resource Acquisition Is Initialisation).
  + For chroot_directory, chroot_fs_union must come before chroot_plain
    in the inheritance and initialisation order.  It's overriding it,
    so needs to follow.
  + It does not take into account the changes made in
    http://git.debian.org/?p=users/rleigh/schroot.git;a=shortlog;h=refs/heads/fs-union
    - changes to maintainer scripts
      + set ±e changes and returning from functions rather than dying
        on error.  This is the rule: all errors are fatal, all scripts
        are idempotent on cleanup.  If there's an error, set -e will
        kill the script and then schroot will clean up.  This is
        simple, robust and foolproof.
      + unnecessary use of "x" != "x$FOO".  You're quoting, so the 'x'
        is not needed, and you're not using C, so don't swap the order
        to prevent assignment.  '"$FOO" != ""' is fine, and '-n "$FOO"
        even better (or -z with appropriate logic).
    - use of dynamic cast in sbuild-session for chroot_plain.  String
      comparisons are slower than dynamic cast and are *not* type-safe.
      dynamic_cast uses pointer comparisons on typeinfo objects.  The
      strings are for user output only; the programmer should use the
      C++ type system correctly.
    - underlay directories.  I don't care about the name, but they need
      to be configurable by the script and schroot.
    - there are other changes as well which I would appreciate if you
      could look over.
  + The changes to run_parts break compatiblity with run-parts(8).  Note
    not running scripts which were not run before failure is a bad plan
    (see above regarding idempotency).  setup scripts may be run with
    setup-stop *multiple time* and there may also be *multiple failures*.
    This is the reason behind the idempodency requirement.  We don't know
    where the failure occured, especially if the cleanup fails, so we
    *guarantee* that the setup-stop action may be run multiple times.
    Scripts not providing this guarantee are broken.

I hope this is useful.  I'll be able to take another look in about a week.


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/20090601/3e262456/attachment.pgp>