[buildd-tools-devel] [PATCH 09/11] [chroot-source] Cleanup clonable handling

[Roger Leigh]

On Mon, Aug 10, 2009 at 08:12:18PM +0200, Jan-Marek Glogowski wrote:
> Currently we have a facet "source", a facet "source-clonable",
> which also has a "source_clonable" member and a get/set pair -
> to much redundant information about the clonable status.
> 
> As a solution we define: the facet "source" is always clonable.
> (a non-clonable source doesn't make any sense).
> 
> Therefore a chroot with the source face is clonable and a
> cloned chroot can simply drop the facet if it's not clonable
> itself.
> 
> As a result this patch renames the facet "source-clonable" to
> "source" and fixes all clonable handling.

Your comment about the get_source_clonable is I think valid.  It
is no longer required and can be removed, which I will do shortly.

I haven't applied this change, because I'm not sure I understand why
the design here is a problem.  I'll explain why it's done this way
in case that wasn't clear from the changelog:

After creation of a chroot object from a keyfile, we can use it
(start a session with clone_session) or if it supports it, we can
create a source chroot object from it (clone_source) which can
then itself be used to create a session.

In the old (pre-facet) way of doing things, there were only two
objects:

  1) chroot
  2) source

with the following clone operation:

  chroot → source

With the addition of facets, this was also generalised to sessions,
so we now have three objects:

  1) chroot
  2) source
  3) session (created from chroot or source)

with the following clone operations

  chroot → session
  chroot → source → session

Not all chroots support cloning of sessions, and not all chroots
support cloning of source chroots.  As a result, we need to know
if a given chroot has the /potential/ to become a source and/or
a session; that is, if the clone_source and clone_session methods
will work.  Additionally, we need to know if a given chroot /is/
a source chroot or a session chroot.

Previously we had a single session and source facet classes, and they
tracked that state.  However, from all of the conditionals in e.g.
get_session_flags, keyfile and other methods, it's clear that we have
two entirely separate classes here: one to handle everything /before/
the source or session chroot is cloned, and one /after/ the cloning
has been done.  Separating them makes the logic clean and unambigious.
Plus, we can tell what an object supports merely by checking for the
presence of the facet; the facet itself doesn't need to keep any
additional state since it's single-purpose.

Looking at the facets through the clone operations, indicated in
square brackets:

  chroot [source_clonable session_clonable]
    → session [session]
  chroot [source_clonable session_clonable]
    → source [source session_clonable]
        → session [session]

As I hope is clear, the presence or absence of the appropriate
facets at each stage control which operations are supported on
the object and how it behaves.

Also note that previously session activation did not clone a
new chroot object: it altered the existing one.  Here, each
transition is creating a new copy of the object, and then
modifying it to reflect the new state.  All state is in the
presence of absence of particular facets, not in the facet
member data.


I hope this clears up any potential confusion, and shows the
intent behind this design.  With this scheme there shouldn't
be any duplication of functionality, since the foo and
foo_clonable facets do completely different things.


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/20090813/1fea12fd/attachment.pgp>