[MERGE] Chroot transport overhaul

[Robert Collins]

On Wed, 2007-03-28 at 14:03 +1000, Martin Pool wrote:
> On 3/28/07, Robert Collins <robertc at robertcollins.net> wrote:
> > Rewritten chroot transport that prevents accidental chroot escapes when
> > using urlutils against the transport's url.(Robert Collins, Andrew Bennetts)
> 
> I think reviews can go more efficiently if you say more about what you
> are changing and how.  A few sentences would do.
>
> Then we can just check that the design is reasonable, and that you
> implemented what you said, rather than inferring the design from a
> multi-hundred-line patch.

Fair enough. 'Change chrooting to use a keyed protocol for every chroot
rather than a single chroot protocol that attempts to 'get it right' -
and fails.

> +    * Rewritten chroot transport that prevents accidental chroot escapes when
> +      using urlutils against the transport's url.
> +      (Robert Collins, Andrew Bennetts)
> 
> You changed the urls, as well as just changing the implementation, I think?

Continuing the meta-discussion: I don't know what you are really asking
here - the implication is that you aren't happy with the NEWS, but there
isn't enough information here for me to correct it without another round
trip. (Martin says this was oversight not slack reviewing).

How about:
 Change the format of chroot urls so that they can be safely manipulated
by generic url utilities without causing the resulting urls to have
escaped the chroot. A side effect of this is that creating a chroot
requires an explicit action using a ChrootServer.

> -        if isinstance(current_transport, chroot.ChrootTransportDecorator):
> -            raise TestSkipped("ChrootTransportDecorator disallows clone('..')")
> 
> Removing the duplication is nice, but what was it doing before and why
> can it be removed?

It was skipping tests that the old chroot code incorrectly failed on,
which we had thought were correct at the time, because it had a
different behaviour on clone('..') at the root of the chroot to other
transports behaviour of clone('..') at the root.



> +    """Server for chroot transports."""
> 
> "Why would you need a server for chroots?"  Can you add a sentence
> saying what it's actually for?

Yes.




> +
> +    def setUp(self):
> +        self.scheme = 'chroot-%d:///' % id(self)
> +        register_transport(self.scheme, self._factory)
> +
> +    def tearDown(self):
> +        unregister_transport(self.scheme, self._factory)
> 
> Treating them all as new schemes seems a bit ugly, couldn't we just
> register the pattern once and treat the id as a parameter, or maybe
> put it in the hostname field?

I don't understand the objection here - memory:// uses the exact same
pattern, and its less code to reuse our existing facility for
multiplexing than writing a new one specific to chroot.

-Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070328/78da0ccd/attachment-0001.pgp