Code review comment for lp:~thumper/juju-core/kvm-manager-start-stop

I'll push the diff once I have fixed the first one. It seems a bit
screwed up right now.

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go
File container/kvm/container.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode16
container/kvm/container.go:16: started *bool
On 2013/11/21 23:57:26, wallyworld wrote:
> Perhaps a comment explaining why a pointer? ie inband signalling of
unknown
> state

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode36
container/kvm/container.go:36: bridge := ""
On 2013/11/21 23:57:26, wallyworld wrote:
> I'd prefer var bridge string here because "" is not a valid value

Changed it, but "" is a technically valid value. If it isn't set, we
don't pass the --bridge value down to the underlying uvt-kvm call.

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode60
container/kvm/container.go:60: if c.IsRunning() {
On 2013/11/21 23:57:26, wallyworld wrote:
> Perhaps
> id !c.isRunning() {
> logger.Debugf(...)
> return nil
> }
> other stuff....

> because the other stuff is more lines of code and I think it reads
better that
> way

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm.go
File container/kvm/kvm.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm.go#newcode78
container/kvm/kvm.go:78: return nil, err
On 2013/11/21 23:57:26, wallyworld wrote:
> Generally, inside these sorts of methods implementing an interface, I
think we
> have been doing stuff like
> return nil, fmt.Errorf("doing blah blah: %v", err)

> to give some content to the final error message, rather that just
return nil,
> err

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm.go#newcode84
container/kvm/kvm.go:84: return nil, err
On 2013/11/21 23:57:26, wallyworld wrote:
> ditto

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm_test.go
File container/kvm/kvm_test.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm_test.go#newcode24
container/kvm/kvm_test.go:24:
On 2013/11/21 23:57:26, wallyworld wrote:
> Aren't there tests we can do for start, stop etc when using mocks?

Yes, and we actually do already. This comment was to remind myself to
implement the live tests, which I did.

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go
File container/kvm/libvirt.go (right):

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode5
container/kvm/libvirt.go:5:
On 2013/11/21 23:57:26, wallyworld wrote:
> Could we add some docstring saying what this file does and also what
packages it
> depends on being installed?

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode22
container/kvm/libvirt.go:22: func run(command string, args ...string)
(output string, err error) {
On 2013/11/21 23:57:26, wallyworld wrote:
> This looks like pretty generic boilerplate. Shouldn't we put this in a
utils
> package somewhere?

Possibly, but it did provide me a very place to ramp up the logging to
test what was going on. I'd be a little loathed to remove that.

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode53
container/kvm/libvirt.go:53: UserData string
On 2013/11/21 23:57:26, wallyworld wrote:
> Could we rename this to UserDataFile cause that's what it is and it
could be
> confused for the actual user data content otherwise. That's what I
thought till
> I read the rest of the code.

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode107
container/kvm/libvirt.go:107: // Perhaps regex matching is the easiest
way to match the lines.
On 2013/11/21 23:57:26, wallyworld wrote:
> s/Perhaps//

Done.

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode112
container/kvm/libvirt.go:112: machineStatus :=
machineListPattern.ExpandString(nil, "$hostname $status", output, s)
On 2013/11/21 23:57:26, wallyworld wrote:
> machineStatus is confusing as a variable name, since it could look
like is is
> being used to store the value of "status" from "id hostname status"
mentioned
> above. Well, that's where my brain went at first. Perhaps
"parsedListOutput" or
> something?

Done.

https://codereview.appspot.com/30020044/