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

Roger Leigh rleigh at codelibre.net
Tue Mar 31 23:43:04 UTC 2009


On Mon, Mar 30, 2009 at 12:05:50AM +0200, Jan-Marek Glogowski wrote:
> 
> On Sun, 29 Mar 2009, Roger Leigh wrote:
> 
> > 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.
> >
> > ==> Rejected in its current form.
> 
> My basic idea was to skip invalid chroots, instead of failing. Especially
> in combination with patch 11.

I have no objection to the aim of the patch here.  It just needs
to avoid catching errors it should leave to the higher-level
handlers, or else genuine errors we should abort right now on
will be ignored.


> > [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?
> 
> I've already split the patch in the three parts: drop mount_device, add
> countainer, unify container handling and drop "special" handling.

I saw the separate mails for that, thanks.  I've applied the first of
the three so far.

> At the beginning I was confused by the used paths:
> 
> - mount_device
> - path
> - location
> - file, device, (location which is actually the directory)
> - mount_location
> 
> I had a look at the code and from my POV, every chroot was doing the same
> - add a private object, r/w the keyfile value add environment for scripts.
> 
> I added the path text to the beginning of the chroot class. Container was
> the my idea to name "the place" where the chroot is.

OK, thanks.  I think that while I like a lot of the cleanup work
you have done, I don't really agree that this is a correct
general abstraction or gains much in code clarity.

One thing to consider is that it will restrict all derived chroot
types to having a single container property , and it may be that
it needs multiple properties to describe fully.  For example, I'd
like to add a KVM chroot type, and this will require an image plus
a host of other options in order to start up.

The container code isn't essential for any of the later changes,
so if you don't mind, I'd prefer to merge the remainder of the
union work, and then we can revisit this change later.

> >   - 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.
> 
> Ok - I simply didn't want to add two more functions which just return a
> static string.  For me this looked more compact, but I really don't care.
> I'll add two functions for the keyfile and detail names.

It's certainly more compact.  However, while I do like compactness,
my main goals are security, correctness and maintainability.  If that
means writing more verbose code, I don't mind the extra functions.

> >   - Missing localisation of static strings.
> 
> I'll fix that.

Don't worry about it for now; we can look at this again following
the merge, since it will require some refactoring anyway.

> > [PATCH 11/22] Allow additional verification of chroot containers
> >
> > What is the purpose of statting the container?
> 
> I have some chroots on an external HD, some on my notebook. I wanted to
> omit the chroots and sessions from the standard lists, which are currently
> not available. I have some additional changes, which also move the
> sessions and the overlay directories to the external HD (not ready yet).
> So I can store the sessions for my OOo builds (one is 12 GB) completely on
> the external HD.

OK.

> > 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.
> 
> I'll recheck the --end-session case. As the stat'ing is optional, the main
> program should just omit the stat in case of --end-session.

OK, that sounds promising.  However, I would like do be able to
explicitly turn this off.  For example, if a session is going to
be omitted, we can end it, but we can't list it.  As a result, the
user may be unaware of the hidden but broken sessions.

> > [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?
> 
> --all-sessions shows sessions for chroots, which are invalid. --sessions
> just shows the sessions with stat'ed containers.

I'm not sure I understand the difference here.  So --sessions would
be the same as --all-sessions, but exclude ones failing verification?

I think I would rather have a single additional option such as
--verify (or any better descriptive name you can think of) which would
enable or disable chroot verification for all commands.

> > [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.
> 
> No - it's the other way around. Per default verification is switched on,
> so I just get lists of "valid" chroots and sessions.
> 
> > [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.
> 
> Nothing is broken - the user just gets a warning. If something broke, I
> made a mistake.

OK.

> > Old syntax needs documenting.
> 
> I'll add a sentence to the directory chroot section. I guess a native
> speaker should review it :-)

I've now done this.

> > 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?
> 
> class chroot_plain : public chroot_directory
> 
> Am I missing something?

No, that's my mistake.

For this last patch, I've rewritten it in a slightly different way.
Due to not committing the container patch yet, it adds get/set_directory
functions and a directory member to chroot_directory.  The rest of the
patch is mostly the same.  The get/set_keyfile code is slightly
different; I wasn't sure why you were treating sessions differently:
if they were started before the upgrade to the new schroot, they have
exactly the same checks required as for chroots.  I also added additional
checks for multiple keys.


I'll start to apply the remaining patches as I find time over the next
few days, and we can continue with the remaining patches in this set
above once that's done.


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.



More information about the Buildd-tools-devel mailing list