[buildd-tools-devel] [Buildd-tools-devel] schroot filesystem union support

[Roger Leigh]

On Sat, Mar 28, 2009 at 01:32:48PM +0000, Roger Leigh wrote:
> On Thu, Mar 26, 2009 at 10:13:38PM +0100, Jan-Marek Glogowski wrote:
> > Hi everybody
> > 
> > A few days ago (and a year ago) I sent a large patch for filesystem union
> > support for schroot. It took me a while to split it up. Final diffstat is
> > 
> >  80 files changed, 2486 insertions(+), 739 deletions(-)
> > 
> > This patchset can be sepearated into the following blocks:
> > 
> > 01-07 Autotools cleanup and debianisation
> > 08-14 Minor features (sessions lists and list validation) and
> >       cleanups of schroot code
> 
> I have now committed patches 1-8.  I'm happy with all but a tiny
> minority of the changes, so most changes are committed verbatim
> or with minor whitespace/spelling/grammar cleanup.  Some of the
> changes were already made by me, so were dropped (or just the
> missing bits added).
> 
> These are my comments on my review so far.  If you feel any of my
> understanding of the patches or what I have committed is
> incorrect, please do follow up with an explanation and/or a
> new patch against the current git if needed.

I have now reviewed patches 9-14.  I have more criticism of these
patches, but please don't take my comments as being too negative!
For most of these patches, I don't understand the reasons behind
why you wanted a particular feature adding or change making, so
if you could provide me with a rationale for each change, that
would greatly help me to understand what you are trying to do.

I do have some technical problems with some of the patches, which
I have detailed below.  But, I can be easily persuaded if there's
a good rationale for doing things the way you did them!

I've also written a large number of observations as I went through
the patches.  They are just what ran through my head as I reviewed it,
so if I misunderstood what you were trying to do, they may be wrong.

Thanks,
Roger


fs-union patch set review2
--------------------------

[PATCH 09/22] Don't fail on missing schroot.conf

Putting the config->add calls inside a try block and then catching and
printing out the error is OK from a code correctness POV, but you are
catching some file and chroot parsing errors in addition to the
specific one (file not found) that you are interested in.

I'm seeing things like (with 0600 perms in /etc/schroot/chroot.d):

% bin/schroot/schroot -l
W: No chroots are defined in ‘/etc/schroot/schroot.conf’ or ‘/etc/schroot/chroot.d’

% bin/schroot/schroot -l -v
W: /etc/schroot/chroot.d/squeeze-amd64-sbuild: Failed to open file: Permission denied
W: No chroots are defined in ‘/etc/schroot/schroot.conf’ or ‘/etc/schroot/chroot.d’

The fact that genuine errors are being ignored (and only printed when
in verbose mode) is a problem.  This one should have been caught by
the top-level catch block which would terminate the program with an
nonzero error status.

I don't object to the patch in principle; what it is trying to do is
a good goal.  However, it needs to be much more specific.  I would
suggest passing in a boolean flag to add() to tell it to allow
missing files and simply call log_exception_warning on stat failure.
This will be both simpler and put the "skip file" logic right after
the stat so higher levels of the code don't need to worry about it.
We also only want to skip the one file, schroot.conf.  If we can't
read a file in /etc/schroot.chroot.d or /var/lib/schroot/session,
that's a reason for outright failure and shouldn't be handled here.

This is a simple error, so fixing is just a matter of finding where
incorrect catch is taking place.

==> Rejected in its current form.


[PATCH 10/22] General chroot container member

What is the purpose of the "chroot container"?  What is the problem
you are trying to solve and the reason for doing it this way?

My understanding (guess) of what you are doing here is to unify the
naming used by the different chroot types for the source of the chroot
filesystem.  However, they were originally named differently for a
reason: they are very different and need handling differently in the
scripts; naming them the same could result in doing something
unfortunate if we forget to special case something.  The inheritance
of related types should mean related uses have the same names, but
maybe this isn't always sufficient?  (The naming could certainly be
improved, as you have done for plain/directory later.)

I'm not opposed to changing this, I just want to fully understand the
rationale for the change.

Some other comments:
  - get_chroot_strings is using *pointers*.  schroot is written
    with an extreme degree of caution, and you won't find a bare
    pointer anywhere else.  References are what need using here.
    schroot uses references or smartpointers, never bare pointers.
    You can remove all those NULL checks for a start: references
    can't be NULL.  I know this is pedantic, but this is one reason
    why we haven't had a segfault reported in schroot for several
    years now (bar odd toolchain issues out of our hands).

  - In the code you are sometimes passing NULL in, to only use one or
    two of the values.  I would split up this function into one for
    each value, and have them return a const& to a static class
    member, then there's no copying by value even inside the function.
    We already use get_chroot_name in this manner, so just adding
    additional methods in the same style is both more efficient and
    safer.

  - Missing localisation of static strings.

  - String formatting is using string methods, which is a bit hairy;
    the rest of the codebase uses boost::format and/or stringstreams
    for all string formatting.  Again, this is pedantic, but we avoid
    all string subscripting bugs at a slight expense of efficiency but
    do gain in security since the chance of making mistakes and
    causing a root exploit though incorrectly modifying a string is
    now minimal.

  - You are using get_chroot_strings to get a container key to
    construct an environment variable in setup_env.  This looks
    more complex than the existing practice of setting it in
    the virtual function for each chroot type, though probably is
    easier to extend.  However, we still need the vfunc to chain up
    to the interface class methods, so I'm not sure what we gain here.

  - Are you planning to add "container" as a replacement for
    chroot-specific key names in schroot.conf?

  - Will CHROOT_CONTAINER replace chroot-specific variables in the
    setup scripts?

  - Where does the chroot-specific environment setup get moved to?
    I see e.g. for block-device CHROOT_DEVICE is removed from setup_env,
    but is still used in the 10mount, but it's not set anywhere else.

  - Is a "container" an internal implementation detail which can be
    kept private, or something we have a public API, or something
    we expose to users?

  - Is putting the "container type" in brackets in the --info output
    useful or desirable?  We already have it in the chroot type and
    this could cause problems parsing --info output, since the name
    changes.


This patch is very large, making several changes which can be
separated:

1) addition of get/set container and container member in chroot base class
   I'm happy with this, but not yet sure what it's for.
2) Replacement of chroot-specific get/set methods with virtual
   get/set container methods.  This can be done in several steps:
   - Existing methods call get/set container internally, keeping
     API the same, and remove chroot-specific data members.
   - Replace existing methods with get/set container directly.
     However, I'm not sure why this needs doing, or why get/set
     container needs to be virtual?


==> Rejected pending further explanation.


[PATCH 11/22] Allow additional verification of chroot containers

What is the purpose of statting the container?

WRT LVM container deletion comment:
+       * Not sure what happens with the LVM snapshot device, if the
+       * originating device is gone...

You can't delete the original device while a snapshot is active.

In stat_chroot_containers, you are using "type" to check the
chroot type.  You already have this information in the form
of the chroot typeinfo, so you can just use dynamic_cast<> here
instead of string comparisons.  It's faster and is also type-safe.
You are doing this for LVM snapshots, just not everything else.

One consideration here is how to properly clean up in case one of
these checks fails.  If this is a session we are checking, and
the backing device is gone, we don't want to fail, we need to
clean up the session properly if --end-session was used, so
bailing out or skipping it is not the desired behaviour.

Note: Not used until patch 13.

==> Rejected pending further explanation.


[PATCH 12/22] Add option to just list sessions (-s,--sessions)

What does this do that --all-sessions does not?  --all-sessions
exists to just select sessions, so what is the difference
between this option and --sessions?

==> Rejected pending further explanation.


[PATCH 13/22] Change listing default to verified lists

Contains two parts:
all_used switched from member function to member
- this means the object user needs to know to set the variable,
  exposing internal details they should not need to know about.
- OK in principle, since at the moment we don't expect the
  all_* options to change underneath the user (but this is the
  reason for having a function).

Enables chroot verification, but only if an --all option is used
(why?)
- The --all* functions are used to get the complete list of chroots, and
  this can be for purposes other than simple listing, such as running a
  command in all chroots.  Any action may be performed in an arbitrary
  number of chroots, including those selected with --all*.
- Interface note: --all* should behave in exactly the same way as the
  same chroots selected with multiple --chroot= options.  They are a
  shortcut for selecting a grouping of chroots, nothing more.

==> Rejected in current form


[PATCH 14/22] Remove ambiguity of location key

The configuration parsing code contains a mechanism to support both
deprecated and obsolete keys.  Breaking the configuration file over a
single Debian release is forbidden; it may be replaced and deprecated
now, then obsoleted and removed in Squeeze+1, then the obsolete key
checking can be removed in Squeeze+2.

Old syntax needs documenting.

Double check manpage markup.

Why are set/get keyfile functions added to chroot_directory, when they
are already present in chroot_plain?  Or, at least, should only be
needed in chroot_plain?

==> Acceptable in principle, but needs more checking and rework.


-- 
  .''`.  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/20090329/27ada5b6/attachment.pgp