Issue 30 #129

robertojrojas · 2016-10-18T23:35:05Z

Here is one implementation of the Monitor using the https://github.com/rgbkrk/libvirt-go package.
Let me know if you there is anything you would like me to change.

mdlayher

There's a lot that needs to be cleaned up for this to be merged.

Follow the style of other files in this repository.

Use interfaces instead of swapping out functions in tests.

Only break long lines of code into multiple lines.

mdlayher · 2016-10-19T03:15:20Z

examples/libvirtgo_events/main.go

+)
+
+var (
+	uri        = flag.String("uri", "qemu:///system", `URI to connect to the libvirtd host.`)


Nitpick, use quotes instead of backticks since we don't need quotes inside the string.

mdlayher · 2016-10-19T03:15:33Z

examples/libvirtgo_events/main.go

+
+var (
+	uri        = flag.String("uri", "qemu:///system", `URI to connect to the libvirtd host.`)
+	domainName = flag.String("domainName", "mydomain", "This is the domain to run commands against.")


Remove "This is".

mdlayher · 2016-10-19T03:15:49Z

examples/libvirtgo_events/main.go

+func main() {
+	flag.Parse()
+
+	libvirtGoMonitor := qmp.NewLibvirtGoMonitor(*uri, *domainName)


Let's just go with mon for consistency with other documentation.

mdlayher · 2016-10-19T03:17:40Z

examples/libvirtgo_events/main.go

+		log.Fatalf("Unable to connect: %v\n", err)
+	}
+
+	eventsChans, err := libvirtGoMonitor.Events()


events for consistency with other documentation.

mdlayher · 2016-10-19T03:17:56Z

examples/libvirtgo_run_command/main.go

+)
+
+var (
+	uri        = flag.String("uri", "qemu:///system", `URI to connect to the libvirtd host.`)


Nitpick, use quotes instead of backticks since we don't need quotes inside the string.

mdlayher · 2016-10-19T03:32:23Z

qmp/libvirtgo_linux.go

+
+		// We only support lifecycleEvents for now
+		if lifecycleEvent, ok :=
+			eventDetails.(libvirt.DomainLifecycleEvent); ok {


Move this up on the same line as the assignment. Only break up excessively long lines.

mdlayher · 2016-10-19T03:33:35Z

qmp/libvirtgo_linux.go

+		Seconds      int64 `json:"seconds"`
+		Microseconds int64 `json:"microseconds"`
+	}
+	timestamp := TimeStamp{


Use the existing Timestamp type in this package.

mdlayher · 2016-10-19T03:33:43Z

qmp/libvirtgo_linux.go

+		Microseconds int64 `json:"microseconds"`
+	}
+	timestamp := TimeStamp{
+		int64(now.Second()),


Used keyed struct literals.

mdlayher · 2016-10-19T03:33:48Z

qmp/libvirtgo_linux.go

+	}
+	timestamp := TimeStamp{
+		int64(now.Second()),
+		int64(now.Second() / int(time.Microsecond)),


Used keyed struct literals.

mdlayher · 2016-10-19T03:34:22Z

qmp/libvirtgo_linux_test.go

@@ -0,0 +1,357 @@
+// Copyright 2016 The go-qemu Authors.


All tests need to make use of interfaces instead of swapping out functions at the package level.

robertojrojas · 2016-10-19T14:44:34Z

@mdlayher great points! Specially the use of interfaces for stubbing functions. I honestly didn't like my approach either, but no alternatives came to mind. I'll start working on this tonight.

mdlayher · 2016-10-19T15:09:17Z

Sounds good. Apologies for all the comments; just trying to keep things consistent throughout the repo where possible.

robertojrojas · 2016-10-27T20:11:48Z

@mdlayher any feedback on my latest commits?

This involves some additional changes to overcome breaking changes from go-libvirt. These are the new changes in this revision of go-libvirt since the last release go-qemu has been using: * remove invitation to empty IRC channel * rpc_test: use Libvirt.addStream to add streams * MockLibvirt: deep-copy response before adding ID * monitor socket conn state in initLibvirtComms * libvirttest: use atomic to read "serial" * integration: wait for Disconnected before assert * add CODEOWNERS * Merge pull request digitalocean#152 from clarkwalcott/readme_update_rpc_link * Docs: Update README with working link * expose Libvirt.Disconnected method * Add NetworkUpdateCompat * update libvirt configuration step for new releases * search generated include directory for header files * ci updates (digitalocean#142) * integration tests (digitalocean#141) * take dialer as input instead of already existing connection (digitalocean#140) * support authentication via polkit (digitalocean#138) * fix some stream problems (digitalocean#137) * Merge pull request digitalocean#136 from digitalocean/jenni/fix_potential_segfault * write lock only needed for actual write * rm unneeded nil'ing of reader and writer * socket Disconnect close can race with nil of conn * Merge pull request digitalocean#135 from digitalocean/jenni/update_cgo * run go get golang.org/x/tools/go/internal/cgo@v0.1.1 * Merge pull request digitalocean#134 from digitalocean/jenni/add_socket_type * change socket to pointer ref * Merge branch 'master' of github.com:digitalocean/go-libvirt into jenni/add_socket_type * Merge pull request digitalocean#133 from digitalocean/jenni/fix_event_unsubscribe_race * reorder stream shutdown/delete * fix race in disconnect of event stream cleanup * add socket type to handle socket connection management * Merge pull request digitalocean#132 from digitalocean/jenni/fix_client_deadlock * fix typo and func conciseness * move listen and cleanup into a func * add timeout on waiting for disconnected * move temporary read error check to a function * Libvirt chan socketCleanupDone -> disconnected * fix accidental go fmt of generated files * add connect/disconnect to unit tests * add tests for callback cleanup on connection disconnect * fix rpc lookup test * fix deadlocked clients on lack of libvirt response * add more robust connection error checking * Merge pull request digitalocean#131 from digitalocean/jenni/fix_invalid_test_data * fix incorrect test data * Merge pull request digitalocean#130 from digitalocean/jenni/stream_shutdown * fix missing stream shutdown in lifecycle subscribe * Merge pull request digitalocean#129 from digitalocean/geoff/vet-cleanup * remove travis config file * Clean up some linter errors * Merge pull request digitalocean#125 from dmacvicar/dmacvicar_puberror * Run generators against libvirt 7.2.0 * Unexport From* (ErrorDomain) * golint * go fmt * libvirt.Error: expose only Code and Message * Run generators against libvirt 7.1.0 * go fmt * Make libvirtError public (libvirt.Error) * github actions (digitalocean#128) * internal/event/stream: fixes unsafe queue access (digitalocean#127) * Merge pull request digitalocean#124 from sam-github/check-wrapped-errors * Check wrapped errors for IsNotFound() * Fix two race conditions related to Disconnect (digitalocean#120) * Merge pull request digitalocean#119 from weizhouapache/readme-add-tls-port * README: Add missing tls port in example * README: Add example connect to libvirt via TLS over TCP (digitalocean#118) * Add overload for Connect() to allow other libvirt drivers (digitalocean#117)

Roberto J Rojas and others added 29 commits October 8, 2016 21:52

started libvirt-go monitor

1b2d460

libvirtgo: added new, connect, run, and disconnect

f2b7da5

Merge remote-tracking branch 'upstream/master' into issue_30

8bc24ba

Merge remote-tracking branch 'upstream/master' into issue_30

84266ae

qmp/libvirt-go: start build tag support

316181f

Merge remote-tracking branch 'upstream/master' into issue_30

f8e85b6

qmp/libvirtgo: better code distribution

f675e2d

qmp/libvirtgo: move exported methods to implementation

e0a60fe

qmp/libvirtgo: move domain, uri to implementation

f5d43e3

qmp/libvirtgo: fix pointer bug on Connect, Run methods

b737dda

qmp/libvirtgo: add command example

c4e662b

qmp/libvirtgo: start events notificaiton

bfb4bef

qmp/libvirtgo: compiles on linux

e4d22a6

qmp/libvirtgo: remove doneChan. change log messages.

f92839f

qmp/libvirtgo: fixed return val issue.

85c6ceb

qmp/libvirtgo: fix bugs. add more comments.

d550391

qmp/libvirtgo: change libvirtgo_events example.

bea75d9

qmp/libvirtgo: remove fmt package. add events interval env variable

b35ebff

qmp/libvirtgo: construct event changes.

0a7d859

qmp/libvirtgo: fix bug

50f507f

Merge remote-tracking branch 'upstream/master' into issue_30

06bd4ef

qmp/libvirtgo: prepare for tests.

a4e3825

qmp/libvirtgo: fix typos and bugs.

7acfe55

qmp/libvirtgo: start unit tests

417cec6

qmp/libvirtgo: add more test

daac6ad

qmp/libvirtgo: add getPollInterval tests

4ef4ca4

qmp/libvirtgo: add events OK test

c3c598c

qmp/libvirtgo: add libvirtgo_run_command to README section

f6749c9

qmp/libvirtgo: add libvirtgo_events example to README section

4838752

mdlayher suggested changes Oct 19, 2016

View reviewed changes

Roberto J Rojas and others added 6 commits October 20, 2016 20:02

qmp/libvirtgo: changes PR review

64e311b

qmp/libvirtgo: use interfaces for internal implmentation

bab3bd7

qmp/libvirtgo: fix events test

20740c8

qmp/libvirtgo: add events loop interval parameter

42ad391

qmp/libvirtgo: add libvirtgo to README

aa14b77

Merge remote-tracking branch 'upstream/master' into issue_30

98ca2de

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 30 #129

Issue 30 #129

robertojrojas commented Oct 18, 2016

mdlayher left a comment

mdlayher Oct 19, 2016

mdlayher Oct 19, 2016

mdlayher Oct 19, 2016

mdlayher Oct 19, 2016

mdlayher Oct 19, 2016

mdlayher Oct 19, 2016

mdlayher Oct 19, 2016

mdlayher Oct 19, 2016

mdlayher Oct 19, 2016

mdlayher Oct 19, 2016

robertojrojas commented Oct 19, 2016

mdlayher commented Oct 19, 2016

robertojrojas commented Oct 27, 2016

Issue 30 #129

Are you sure you want to change the base?

Issue 30 #129

Conversation

robertojrojas commented Oct 18, 2016

mdlayher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertojrojas commented Oct 19, 2016

mdlayher commented Oct 19, 2016

robertojrojas commented Oct 27, 2016