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

LTGM with some suggestions.

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
Perhaps a comment explaining why a pointer? ie inband signalling of
unknown state

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode36
container/kvm/container.go:36: bridge := ""
I'd prefer var bridge string here because "" is not a valid value

https://codereview.appspot.com/30020044/diff/1/container/kvm/container.go#newcode60
container/kvm/container.go:60: if c.IsRunning() {
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

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
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

https://codereview.appspot.com/30020044/diff/1/container/kvm/kvm.go#newcode84
container/kvm/kvm.go:84: return nil, err
ditto

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:
Aren't there tests we can do for start, stop etc when using mocks?

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:
Could we add some docstring saying what this file does and also what
packages it depends on being installed?

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) {
This looks like pretty generic boilerplate. Shouldn't we put this in a
utils package somewhere?

https://codereview.appspot.com/30020044/diff/1/container/kvm/libvirt.go#newcode53
container/kvm/libvirt.go:53: UserData string
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.

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.
s/Perhaps//

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)
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?

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