Code review comment for lp://staging/~themue/golxc/001-added-sudo-and-first-test
Revision history for this message
| Roger Peppe (rogpeppe) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
513534c
demo site
(Get the code!)
looks good to me apart from (still) the error messages.
a few minor suggestions in addition to that.
https:/
File golxc.go (right):
https:/
golxc.go:219: switch len(states) {
On 2012/11/22 14:35:57, fwereade wrote:
> I'm not sure the switch is a big win over a simple err return when no
states are
> supplied -- the one-state case may be more efficient but I'm not sure
it really
> aids clarity. Up to you though.
i think i agree here. we don't mind about a single non-escaping
allocation. i'd lose the "case 1" and make the "case 0" into an if that
returns the error.
https:/
golxc.go:271: // String returns a brief information about the container.
On 2012/11/22 14:35:57, fwereade wrote:
> s/a //
> ?
s/a brief // ?
https:/
golxc.go:277: return fmt.Sprintf(
c.name, state, pid)
i'd be tempted to go a little more terse than this.
something like this, as an example, perhaps?
container "foo" (STARTING, pid 2353)
https:/
golxc.go:304: return outbuf.String(), errbuf.String(), err
i'm still not happy with this. if we call Container.Start and get the
error "exit status 1", that's an extremely unhelpful error. for an
example of what might be a reasonable approach, see how lbox deals with
error returns from running bzr. (search for bzrError in the lbox
source). you might want to tailor that for the way that lxc tends to
print error messages.
https:/
File golxc_test.go (right):
https:/
golxc_test.go:12: type LXCSuite struct{}
i wonder if it might be worth Skipping the suite (with a warning,
perhaps) if we're not running as root.
https:/