Re: [libvirt] [PATCH 00/12] Domain Events - Round 2

[Jim Meyering]

Ben Guthro [EMAIL PROTECTED] wrote:
 The following patch series implements the Events API discussed here 
 previously in this thread:
 http://www.redhat.com/archives/libvir-list/2008-September/msg00321.html and
 http://www.redhat.com/archives/libvir-list/2008-October/msg00245.html
...

Nice work.
I spotted no problems in review, and noticed only one when testing:
In the new file, examples/domain-events/events-c/Makefile.am

 diff --git a/examples/domain-events/events-c/Makefile.am 
 b/examples/domain-events/events-c/Makefile.am
...
 @@ -0,0 +1,5 @@
 +INCLUDES = [EMAIL PROTECTED]@/include
 +bin_PROGRAMS = event-test
 +event_test_CFLAGS = $(WARN_CFLAGS)
 +event_test_SOURCES = event-test.c
 +event_test_LDADD = [EMAIL PROTECTED]@/src/.libs -lvirt

that would fail with a non-srcdir build (i.e., when the
build_dir != srcdir), because then @top_srcdir@/src/.libs
doesn't exist, and that causes make distcheck to fail.

Also, it is best not to reference libtool's .libs/ directories
directly, so you can use this instead:

diff --git a/examples/domain-events/events-c/Makefile.am 
b/examples/domain-events/events-c/Makefile.am
index 1788dac..3c63ca3 100644
--- a/examples/domain-events/events-c/Makefile.am
+++ b/examples/domain-events/events-c/Makefile.am
@@ -2,4 +2,4 @@ INCLUDES = [EMAIL PROTECTED]@/include
 bin_PROGRAMS = event-test
 event_test_CFLAGS = $(WARN_CFLAGS)
 event_test_SOURCES = event-test.c
-event_test_LDADD = [EMAIL PROTECTED]@/src/.libs -lvirt
+event_test_LDADD = @top_builddir@/src/libvirt.la

Finally, I noticed that make check does not run the new
event-test program that's built there.  Should it?

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/12] Domain Events - Round 2

[Daniel P. Berrange]

On Fri, Oct 17, 2008 at 11:51:51AM -0400, Ben Guthro wrote:
 The following patch series implements the Events API discussed here 
 previously in this thread:
 http://www.redhat.com/archives/libvir-list/2008-September/msg00321.html and
 http://www.redhat.com/archives/libvir-list/2008-October/msg00245.html
 
 By Daniel B's advice - I have broken this patch into the following:

Thanks that has made it very nice to review !

In a quick 5 minute read through, I don't see any major issues that
stand out to me - architecturally I looks pretty much exactly as I
had imagined it would. I'll do a more detailed review over the weekend
so there's some feedback for monday.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list