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

[Jan-Marek Glogowski]

Thanks for the quick review.

On Sun, 29 Mar 2009, Roger Leigh wrote:

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

My basic idea was to skip invalid chroots, instead of failing. Especially
in combination with patch 11.

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

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.

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

No it's just the internal handling, as every chroot was doing the same.
But for the user it's still the old style (I couldn't think of a better
name then get_chroot_strings).

The only exception is the directory chroot, when I renamed location ->
directory and deprecated location. All config should still work.

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

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.

>   - Missing localisation of static strings.

I'll fix that.

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

No objections - I wasn't aware. Easy to change.

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

No. I never thought of that. I just saw every chroot doing the same for
keyfile and detail handling and sometimes additional checks in the setter
function.

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

No - I didn't came up with a good use case yet, except the stat, which
finally moved into patch 11.

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

It's set by the setup_env of sbuild::chroot. Basically I'm uppercasing the
keyfile key value.

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

My main usage for the container is patch 11.

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

Seems I added the "Directory" detail, but forgot to readd the others in
the chroot::get_details. From the point of the cleanup it would be better
to add container parsing to sbuild. But anyway it's easy to add the
details again.

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

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

Never used LVM, so this was just a guess.

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

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.

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

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

> Old syntax needs documenting.

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

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

Jan-Marek