code review 6789043: os/exec: fix fd leak on error

os/exec: fix possible data loss and fd leak

The reasonable-seeming code below could lose data and it
will not terminate if the command does not exist.

It can lose data because the client side of the pipe
is closed after Wait returns, even if we haven't read
all the data yet.

It won't terminate when the command is not found because
the server side of the pipe is not closed by Run if the command
was not found, so the io.Copy never gets EOF.

        func getOutput(cmd string) (string, error) {
                c := exec.Command(cmd)
                r, err := c.StdoutPipe()
                if err != nil {
                        return "", err
                }
                done := make(chan bool, 1)
                var buf bytes.Buffer
                go func() {
                        io.Copy(&buf, r)
                        r.Close()
                        done <- true
                }()
                err = c.Run()
                <-done
                return buf.String(), err
        }

This CL fixes the first issue above by making sure that
the server side of the pipe is always closed
when Start is called, even if Start returns an error.

It fixes the second issue by not closing the client
side of the pipe at all, and leaving it up to the
code that calls StdoutPipe to close it for itself.
It could be argued that this breaks the Go 1 contract,
but I think it's a bug that needs fixing.

Fixes issue 4290.

[rog, 2012-10-26 10:29:35]

Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rog, 2012-10-26 14:15:17]

On 26 October 2012 11:29,  <rogpeppe@gmail.com> wrote:
> Related aside:
>
> I think it's a mistake that the ReadCloser returned by
> StdoutPipe and StderrPipe is closed after Wait is called.
> In particular, it's sometimes useful to call Cmd.Run and
> have an independent goroutine reading the output pipe,
> but if the reader side of the pipe is closed after Wait, the
> Wait must be called after the reader completes, compromising
> the reader's independence.
>
> A more succinct argument: if something gives me a ReadCloser,
> it's usual that I'm the one expected to close it.

I created issue 4290 to track this.

[bradfitz, 2012-10-26 15:05:49]

Your CL description subject doesn't really match the CL description body
and code.

It's more like:

    os/exec: move where things are cleaned up failure

I think most people are going to check the return value from Start before
doing other stuff, so how we have it now probably works more often.

Changing this feels like it'll just surprise a different set of people.

With this CL, you're just making the reader get a different error instead
of EOF?  That seems acceptable in an error case?


On Fri, Oct 26, 2012 at 3:29 AM, <rogpeppe@gmail.com> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> os/exec: fix fd leak on error
>
> This is by no means the only possible solution.
> Another possibility would be to cause StdoutPipe
> to fail if c.err is not nil. I'm not keen on that because
> means the error is now seen in an unexpected
> place.
>
> I think the "bad file descriptor" check is probably bogus
> because it's OS specific. Better suggestions
> welcome.
>
> Related aside:
>
> I think it's a mistake that the ReadCloser returned by
> StdoutPipe and StderrPipe is closed after Wait is called.
> In particular, it's sometimes useful to call Cmd.Run and
> have an independent goroutine reading the output pipe,
> but if the reader side of the pipe is closed after Wait, the
> Wait must be called after the reader completes, compromising
> the reader's independence.
>
> A more succinct argument: if something gives me a ReadCloser,
> it's usual that I'm the one expected to close it.
>
> Whether we could/should fix this is another matter.
>
> Please review this at
http://codereview.appspot.com/**6789043/<http://codereview.appspot.com/6789043/>
>
> Affected files:
>   M src/pkg/os/exec/exec.go
>   M src/pkg/os/exec/exec_test.go
>
>
> Index: src/pkg/os/exec/exec.go
> ==============================**==============================**=======
> --- a/src/pkg/os/exec/exec.go
> +++ b/src/pkg/os/exec/exec.go
> @@ -227,6 +227,7 @@
>  // returned for I/O problems.
>  func (c *Cmd) Run() error {
>         if err := c.Start(); err != nil {
> +               c.closeDescriptors(c.**closeAfterWait)
>                 return err
>         }
>         return c.Wait()
> @@ -235,6 +236,7 @@
>  // Start starts the specified command but does not wait for it to
> complete.
>  func (c *Cmd) Start() error {
>         if c.err != nil {
> +               c.closeDescriptors(c.**closeAfterStart)
>                 return c.err
>         }
>         if c.Process != nil {
> Index: src/pkg/os/exec/exec_test.go
> ==============================**==============================**=======
> --- a/src/pkg/os/exec/exec_test.go
> +++ b/src/pkg/os/exec/exec_test.go
> @@ -93,6 +93,42 @@
>         }
>  }
>
> +func TestPipeWriterClosedAfterStart**Error(t *testing.T) {
> +       c := Command("non-existent command")
> +       r, err := c.StdoutPipe()
> +       if err != nil {
> +               t.Fatalf("cannot make stdout pipe: %v", err)
> +       }
> +       c.Start()
> +       _, err = r.Read(make([]byte, 10))
> +       if err != io.EOF {
> +               t.Fatalf("got %q, want io.EOF", err)
> +       }
> +}
> +
> +func testPipeClosedAfterRun(t *testing.T, c *Cmd) {
> +       r, err := c.StdoutPipe()
> +       if err != nil {
> +               t.Fatalf("cannot make stdout pipe: %v", err)
> +       }
> +       c.Run()
> +       _, err = r.Read(make([]byte, 10))
> +       if err == nil {
> +               t.Fatalf("got no error, want bad fd")
> +       }
> +       if !strings.HasSuffix(err.Error()**, "bad file descriptor") {
> +               t.Fatalf("got %q, want bad file descriptor error", err)
> +       }
> +}
> +
> +func TestPipeCloseAfterRun(t *testing.T) {
> +       c := helperCommand("exit", "0")
> +       testPipeClosedAfterRun(t, c)
> +
> +       c = Command("non-existent command")
> +       testPipeClosedAfterRun(t, c)
> +}
> +
>  func TestPipes(t *testing.T) {
>         check := func(what string, err error) {
>                 if err != nil {
>
>
>

[rog, 2012-10-26 15:41:28]

On 26 October 2012 16:04, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Your CL description subject doesn't really match the CL description body and
> code.
>
> It's more like:
>
>     os/exec: move where things are cleaned up failure
>
> I think most people are going to check the return value from Start before
> doing other stuff, so how we have it now probably works more often.
>
> Changing this feels like it'll just surprise a different set of people.
>
> With this CL, you're just making the reader get a different error instead of
> EOF?  That seems acceptable in an error case?

I'm not sure that helps. StdoutPipe must be called before Start, so
checking Start's error return is not useful in fixing this issue AFAICS.
If I have this code (which I think is reasonably idiomatic):

    c := exec.Command("some-command-that-does-not-exist")
    r, err := c.StdoutPipe()
    if err != nil {
        return err
    }
    err := c.Run()
    if err != nil {
        return err
    }
    io.Copy(dest, r)


then:

a) the client side of the stdout pipe will not be closed (the user
must close it explicitly
if run fails, which actually i think is fine, because i think the user should
*always* close it, as per issue 4290, but breaks likely current assumptions).

b) the server side of the stdout pipe is not closed and cannot
be closed by the user except by doing something like
c.Stdout.(io.ReadCloser).Close() which seems wrong to me.

as an example, the only instance in the Go core code that uses StdoutPipe,
net/http/cgi, will be vulnerable to this leak AFAICS.

[rog, 2012-10-30 11:00:24]

PTAL.

I've changed the scope of this issue to fix another closely
related issue at the same time.

[rog, 2012-10-30 11:44:21]

On 30 October 2012 11:00,  <rogpeppe@gmail.com> wrote:
> PTAL.
>
> I've changed the scope of this issue to fix another closely
> related issue at the same time.

hmm, this is still wrong, sorry.

[bradfitz, 2012-10-30 11:57:38]

Okay, this makes more sense now with the updated CL description and tests.

I wonder whether, for Go 1 compatibility reasons, we should have the StdoutPipe
and StderrPipe io.ReadCloser close automatically (but once) on Read EOF.  An EOF
will Close (and release the pipe fd), but another explicit Close will be a
no-op.

https://codereview.appspot.com/6789043/diff/11001/src/pkg/os/exec/exec_test.go
File src/pkg/os/exec/exec_test.go (right):

https://codereview.appspot.com/6789043/diff/11001/src/pkg/os/exec/exec_test.g...
src/pkg/os/exec/exec_test.go:105: t.Fatalf("start succeeded unexpectedly")
s/Fatalf/Fatal/

https://codereview.appspot.com/6789043/diff/11001/src/pkg/os/exec/exec_test.g...
src/pkg/os/exec/exec_test.go:108: t.Fatalf("server side of pipe was not closed")
Fatal

https://codereview.appspot.com/6789043/diff/11001/src/pkg/os/exec/exec_test.g...
src/pkg/os/exec/exec_test.go:131: <-done
select {
case <-done:
case <-time.After(10 * time.Second):
   t.Fatal("timeout waiting for copy goroutine")
}

[rog, 2012-10-30 13:22:52]

On 30 October 2012 11:57,  <bradfitz@golang.org> wrote:
> Okay, this makes more sense now with the updated CL description and
> tests.
>
> I wonder whether, for Go 1 compatibility reasons, we should have the
> StdoutPipe and StderrPipe io.ReadCloser close automatically (but once)
> on Read EOF.  An EOF will Close (and release the pipe fd), but another
> explicit Close will be a no-op.

I'm not sure we can do that without making StdoutPipe return something
that's not of type *os.File, and I suspect that really would break things
(and, worse, at run time not compile time)

The reason for my "still wrong" comment above is that removing closeAfterWait
means that if a setupFd fails, it can leak earlier file descriptors
created by setupFd.

The other issue that still concerns me is the following:

    c := exec.Command("something")
    r0, err := c.StdoutPipe()
    if err != nil {
         return
    }
    defer r0.Close()
    r1, err := c.StderrPipe()
    if err != nil {
        return
    }
    defer r1.Close()
    err = c.Start()
    if err != nil {
         return
    }

I think we want the above code to be leak free in all error
cases, but it's not - if StdoutPipe succeeds but StderrPipe
fails then we'll leak the server side of the stdout pipe.

I *think* that the only decent solution to this is to change
each *Pipe method so that
a) if there's already been an error, it fails
b) when it fail, it closes all server-side pipe ends.

This would result in one significant alteration in semantics - the
error from a non-existent executable would be detected earlier
than currently.

I suspect there are probably nicer solutions though.

[rog, 2012-11-12 19:59:03]

ping

On 30 October 2012 13:22, roger peppe <rogpeppe@gmail.com> wrote:
> On 30 October 2012 11:57,  <bradfitz@golang.org> wrote:
>> Okay, this makes more sense now with the updated CL description and
>> tests.
>>
>> I wonder whether, for Go 1 compatibility reasons, we should have the
>> StdoutPipe and StderrPipe io.ReadCloser close automatically (but once)
>> on Read EOF.  An EOF will Close (and release the pipe fd), but another
>> explicit Close will be a no-op.
>
> I'm not sure we can do that without making StdoutPipe return something
> that's not of type *os.File, and I suspect that really would break things
> (and, worse, at run time not compile time)
>
> The reason for my "still wrong" comment above is that removing closeAfterWait
> means that if a setupFd fails, it can leak earlier file descriptors
> created by setupFd.
>
> The other issue that still concerns me is the following:
>
>     c := exec.Command("something")
>     r0, err := c.StdoutPipe()
>     if err != nil {
>          return
>     }
>     defer r0.Close()
>     r1, err := c.StderrPipe()
>     if err != nil {
>         return
>     }
>     defer r1.Close()
>     err = c.Start()
>     if err != nil {
>          return
>     }
>
> I think we want the above code to be leak free in all error
> cases, but it's not - if StdoutPipe succeeds but StderrPipe
> fails then we'll leak the server side of the stdout pipe.
>
> I *think* that the only decent solution to this is to change
> each *Pipe method so that
> a) if there's already been an error, it fails
> b) when it fail, it closes all server-side pipe ends.
>
> This would result in one significant alteration in semantics - the
> error from a non-existent executable would be detected earlier
> than currently.
>
> I suspect there are probably nicer solutions though.

[bradfitz, 2012-11-12 23:57:35]

On Tue, Oct 30, 2012 at 6:22 AM, roger peppe <rogpeppe@gmail.com> wrote:

> On 30 October 2012 11:57,  <bradfitz@golang.org> wrote:
> > Okay, this makes more sense now with the updated CL description and
> > tests.
> >
> > I wonder whether, for Go 1 compatibility reasons, we should have the
> > StdoutPipe and StderrPipe io.ReadCloser close automatically (but once)
> > on Read EOF.  An EOF will Close (and release the pipe fd), but another
> > explicit Close will be a no-op.
>
> I'm not sure we can do that without making StdoutPipe return something
> that's not of type *os.File, and I suspect that really would break things
> (and, worse, at run time not compile time)
>

Well, we never promised in the go1 contract what the underlying type is.
 And it's possible for code to be updated to work with both go1 and go1.1
by doing a comma-ok type assertion, so it's not as bad as real API changes
where code won't compile in both.


> The reason for my "still wrong" comment above is that removing
> closeAfterWait
> means that if a setupFd fails, it can leak earlier file descriptors
> created by setupFd.


> The other issue that still concerns me is the following:
>
>     c := exec.Command("something")
>     r0, err := c.StdoutPipe()
>     if err != nil {
>          return
>     }
>     defer r0.Close()
>     r1, err := c.StderrPipe()
>     if err != nil {
>         return
>     }
>     defer r1.Close()
>     err = c.Start()
>     if err != nil {
>          return
>     }
>
> I think we want the above code to be leak free in all error
> cases, but it's not - if StdoutPipe succeeds but StderrPipe
> fails then we'll leak the server side of the stdout pipe.
>
> I *think* that the only decent solution to this is to change
> each *Pipe method so that
> a) if there's already been an error, it fails
> b) when it fail, it closes all server-side pipe ends.
>
> This would result in one significant alteration in semantics - the
> error from a non-existent executable would be detected earlier
> than currently.
>

Why?  Just close all the write sides after the Start fails?


>
> I suspect there are probably nicer solutions though.
>

[rog, 2012-11-13 11:43:31]

On 12 November 2012 23:57, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>
> On Tue, Oct 30, 2012 at 6:22 AM, roger peppe <rogpeppe@gmail.com> wrote:
>>
>> On 30 October 2012 11:57,  <bradfitz@golang.org> wrote:
>> > Okay, this makes more sense now with the updated CL description and
>> > tests.
>> >
>> > I wonder whether, for Go 1 compatibility reasons, we should have the
>> > StdoutPipe and StderrPipe io.ReadCloser close automatically (but once)
>> > on Read EOF.  An EOF will Close (and release the pipe fd), but another
>> > explicit Close will be a no-op.
>>
>> I'm not sure we can do that without making StdoutPipe return something
>> that's not of type *os.File, and I suspect that really would break things
>> (and, worse, at run time not compile time)
>
>
> Well, we never promised in the go1 contract what the underlying type is.
> And it's possible for code to be updated to work with both go1 and go1.1 by
> doing a comma-ok type assertion, so it's not as bad as real API changes
> where code won't compile in both.

My concern here is that we'd be adding this type for compatibility reasons,
but we can break code as well as fixing it.

However, after looking at a moderately representative example of code
out there using StdoutPipe (I downloaded all the packages mentioned in
go.pkgdoc.org)
I think you're right - I don't see any code type-asserting to *os.File, but
I've seen quite a few places that will leak if we don't close
it automatically. I've also seen quite a few places that have bugs that
will be fixed by this CL.

>> The reason for my "still wrong" comment above is that removing
>> closeAfterWait
>> means that if a setupFd fails, it can leak earlier file descriptors
>> created by setupFd.
>>
>>
>> The other issue that still concerns me is the following:
>>
>>     c := exec.Command("something")
>>     r0, err := c.StdoutPipe()
>>     if err != nil {
>>          return
>>     }
>>     defer r0.Close()
>>     r1, err := c.StderrPipe()
>>     if err != nil {
>>         return
>>     }
>>     defer r1.Close()
>>     err = c.Start()
>>     if err != nil {
>>          return
>>     }
>>
>> I think we want the above code to be leak free in all error
>> cases, but it's not - if StdoutPipe succeeds but StderrPipe
>> fails then we'll leak the server side of the stdout pipe.
>>
>> I *think* that the only decent solution to this is to change
>> each *Pipe method so that
>> a) if there's already been an error, it fails
>> b) when it fail, it closes all server-side pipe ends.
>>
>> This would result in one significant alteration in semantics - the
>> error from a non-existent executable would be detected earlier
>> than currently.
>
>
> Why?  Just close all the write sides after the Start fails?

The problem is that lots of code is similar to the sample
above - if it sees that StdoutPipe fails, it stops there and then,
so Start never has a chance to act.

Take a look at net/http/cgi/host.go:/ServeHTTP for an example.

[bradfitz, 2012-11-13 15:06:17]

On Tue, Nov 13, 2012 at 3:43 AM, roger peppe <rogpeppe@gmail.com> wrote:

> On 12 November 2012 23:57, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> >
> >
> > On Tue, Oct 30, 2012 at 6:22 AM, roger peppe <rogpeppe@gmail.com> wrote:
> >>
> >> On 30 October 2012 11:57,  <bradfitz@golang.org> wrote:
> >> > Okay, this makes more sense now with the updated CL description and
> >> > tests.
> >> >
> >> > I wonder whether, for Go 1 compatibility reasons, we should have the
> >> > StdoutPipe and StderrPipe io.ReadCloser close automatically (but once)
> >> > on Read EOF.  An EOF will Close (and release the pipe fd), but another
> >> > explicit Close will be a no-op.
> >>
> >> I'm not sure we can do that without making StdoutPipe return something
> >> that's not of type *os.File, and I suspect that really would break
> things
> >> (and, worse, at run time not compile time)
> >
> >
> > Well, we never promised in the go1 contract what the underlying type is.
> > And it's possible for code to be updated to work with both go1 and go1.1
> by
> > doing a comma-ok type assertion, so it's not as bad as real API changes
> > where code won't compile in both.
>
> My concern here is that we'd be adding this type for compatibility reasons,
> but we can break code as well as fixing it.
>

It'd be a private type.  If it's too much of a problem for some
yet-unidentified user, we could always add a method on that private type:

func (c *closeOnEOFReader) File() *os.File { ... }

And they could assert for that interface if they really needed the
*os.File.  I'm not proposing that (yet?) though.

>
> However, after looking at a moderately representative example of code
> out there using StdoutPipe (I downloaded all the packages mentioned in
> go.pkgdoc.org)
> I think you're right - I don't see any code type-asserting to *os.File, but
> I've seen quite a few places that will leak if we don't close
> it automatically. I've also seen quite a few places that have bugs that
> will be fixed by this CL.
>

Is this CL done?  It sounds like there's more you want to do.


> >> The reason for my "still wrong" comment above is that removing
> >> closeAfterWait
> >> means that if a setupFd fails, it can leak earlier file descriptors
> >> created by setupFd.
> >>
> >>
> >> The other issue that still concerns me is the following:
> >>
> >>     c := exec.Command("something")
> >>     r0, err := c.StdoutPipe()
> >>     if err != nil {
> >>          return
> >>     }
> >>     defer r0.Close()
> >>     r1, err := c.StderrPipe()
> >>     if err != nil {
> >>         return
> >>     }
> >>     defer r1.Close()
> >>     err = c.Start()
> >>     if err != nil {
> >>          return
> >>     }
> >>
> >> I think we want the above code to be leak free in all error
> >> cases, but it's not - if StdoutPipe succeeds but StderrPipe
> >> fails then we'll leak the server side of the stdout pipe.
> >>
> >> I *think* that the only decent solution to this is to change
> >> each *Pipe method so that
> >> a) if there's already been an error, it fails
> >> b) when it fail, it closes all server-side pipe ends.
> >>
> >> This would result in one significant alteration in semantics - the
> >> error from a non-existent executable would be detected earlier
> >> than currently.
> >
> >
> > Why?  Just close all the write sides after the Start fails?
>
> The problem is that lots of code is similar to the sample
> above - if it sees that StdoutPipe fails, it stops there and then,
> so Start never has a chance to act.
>
> Take a look at net/http/cgi/host.go:/ServeHTTP for an example.
>

That code seems fine.  But you're saying if we'd also called StderrPipe
first, we could leak the write side of StderrPipe if StdoutPipe failed?
 So, sure, add a cmd.cleanup() method called in a few error paths in the
exec package, closing things.

Let me know when you have another version of this patch for review.

[rsc, 2012-11-13 15:22:30]

I agree that the code in the CL description is reasonable sounding, and if we
had more flexibility in the API I'd probably change something. But there is a
good way to work within the current API:

call StdoutPipe
call Start
Read until EOF
call Wait

It doesn't even need a goroutine. All the code changes we can make are API
changes in one form or another. Perhaps the right solution is to make the docs
very clear, by way of an example, what the working way to read from a pipe is,
so that people won't arrive at the "reasonable sounding" version.

[rog, 2012-11-14 08:43:42]

On 13 November 2012 15:22,  <rsc@golang.org> wrote:
> I agree that the code in the CL description is reasonable sounding, and
> if we had more flexibility in the API I'd probably change something. But
> there is a good way to work within the current API:
>
> call StdoutPipe
> call Start
> Read until EOF
> call Wait
>
> It doesn't even need a goroutine. All the code changes we can make are
> API changes in one form or another. Perhaps the right solution is to
> make the docs very clear, by way of an example, what the working way to
> read from a pipe is, so that people won't arrive at the "reasonable
> sounding" version.

Unfortunately the above doesn't work if the started process forks
another process in the background. In that case the Read can
take an indefinite time to return, and for some code (this was
the example that brought me up against this problem) it is
actually the original process exiting that is important.

However, we were able to work around the issue by using
os.Pipe directly, rather than StdoutPipe,  and perhaps that's
how it should be.

What do you think about the leak-on-error issue? Should we document
that it's important to call Start even when a *Pipe method call
fails?

[bradfitz, 2012-11-14 16:59:04]

On Wed, Nov 14, 2012 at 12:43 AM, roger peppe <rogpeppe@gmail.com> wrote:

> On 13 November 2012 15:22,  <rsc@golang.org> wrote:
> > I agree that the code in the CL description is reasonable sounding, and
> > if we had more flexibility in the API I'd probably change something. But
> > there is a good way to work within the current API:
> >
> > call StdoutPipe
> > call Start
> > Read until EOF
> > call Wait
> >
> > It doesn't even need a goroutine. All the code changes we can make are
> > API changes in one form or another. Perhaps the right solution is to
> > make the docs very clear, by way of an example, what the working way to
> > read from a pipe is, so that people won't arrive at the "reasonable
> > sounding" version.
>
> Unfortunately the above doesn't work if the started process forks
> another process in the background. In that case the Read can
> take an indefinite time to return, and for some code (this was
> the example that brought me up against this problem) it is
> actually the original process exiting that is important.
>

Why was the first process leaking stdout to its child?  Sounds like the
real problem.


> What do you think about the leak-on-error issue? Should we document
> that it's important to call Start even when a *Pipe method call
> fails?
>

If StdoutPipe or StderrPipe fails, the program's probably already pretty
screwed (nothing good happens when you're out of fds), so I'm not too
worried.  Especially because the finalizer will close it anyway.  I would
err on the side less esoteric docs.

[rog, 2012-11-14 18:00:46]

On 14 November 2012 16:59, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>> Unfortunately the above doesn't work if the started process forks
>> another process in the background. In that case the Read can
>> take an indefinite time to return, and for some code (this was
>> the example that brought me up against this problem) it is
>> actually the original process exiting that is important.
>
>
> Why was the first process leaking stdout to its child?  Sounds like the real
> problem.

The first process is a shell script written by some third party - we don't
have control over it, but it's important that we finish when the script
finishes.

I suppose this may be a relatively unusual requirement,
and thus justify a special treatment. I didn't think so at the time,
but if we can't change the os/exec API at all, we can live with it.

>> What do you think about the leak-on-error issue? Should we document
>> that it's important to call Start even when a *Pipe method call
>> fails?
>
>
> If StdoutPipe or StderrPipe fails, the program's probably already pretty
> screwed (nothing good happens when you're out of fds), so I'm not too
> worried.  Especially because the finalizer will close it anyway.  I would
> err on the side less esoteric docs.

OK, seems good to me.

I'll abandon the CL and we'll write some docs/examples to point
out the pitfalls.

  cheers,
    rog.