At 12/24/2010 08:25 AM, Eric Blake Write: > On 12/23/2010 01:56 AM, Wen Congyang wrote: >> move daemon/event.* into src/util/ directory because >> the timer needs the API virEventRunOnce(). >> >> Signed-off-by: Wen Congyang <we...@cn.fujitsu.com> >> >> --- >> daemon/Makefile.am | 1 - >> daemon/event.c | 700 >> ---------------------------------------------- >> daemon/event.h | 134 --------- >> daemon/libvirtd.c | 2 +- >> src/Makefile.am | 3 +- >> src/libvirt_private.syms | 14 + >> src/util/event_impl.c | 700 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> src/util/event_impl.h | 134 +++++++++ >> tests/Makefile.am | 2 +- >> tests/eventtest.c | 2 +- >> tools/Makefile.am | 1 - >> tools/console.c | 2 +- >> tools/virsh.c | 2 +- >> 13 files changed, 855 insertions(+), 842 deletions(-) >> delete mode 100644 daemon/event.c >> delete mode 100644 daemon/event.h >> create mode 100644 src/util/event_impl.c >> create mode 100644 src/util/event_impl.h > > If you use 'git config diff.renames true', then this email would have > dropped from about 60k to just under 5k, by showing code motion instead > of delete and recreation. As it is, I find that style easier to review, > so I'm taking the liberty of using it in my reply: > >> +++ b/daemon/libvirtd.c >> @@ -62,7 +62,7 @@ >> #include "uuid.h" >> #include "remote_driver.h" >> #include "conf.h" >> -#include "event.h" >> +#include "event_impl.h" > > Why the rename? Oh, because src/util/event.c already exists. Can we > merge those into one file, rather than adding the _impl variant? Yes, we can merge those into one file, but git diff rename detection does not work if we merge those into one file...
> >> +++ b/src/libvirt_private.syms >> @@ -377,6 +377,20 @@ virEventUpdateHandle; >> virEventUpdateTimeout; >> >> >> +# event_impl.h >> +virEventAddHandleImpl; >> +virEventUpdateHandleImpl; >> +virEventRemoveHandleImpl; >> +virEventAddTimeoutImpl; >> +virEventUpdateTimeoutImpl; >> +virEventRemoveTimeoutImpl; >> +virEventInit; >> +virEventRunOnce; >> +virEventHandleTypeToPollEvent; >> +virPollEventToEventHandleType; >> +virEventInterrupt; > > Keeping this list sorted makes it easier to maintain. OK. > > Wow, now that we're actually exporting these symbol names, I wonder if > it's also time for a bulk rename to drop the Impl suffix. (It's best to > separate function renames into a different patch from file motion, so > that git diff rename detection has a decent chance of compact diff > representations). Also, do all of them need to be in src/util/, or can > some of them still remain in just daemon/ (for example, > virPollEventToEventHandleType is only referenced by daemon/mdns.c and > examples/domain-events/events-c/event-test.c). virPollEventToEventHandleType is referenced by daemon/event.c too. > > For example, maybe all the virEvent APIs should take a virEventPtr as > the first parameter. Then you could add a new function > virEventRegisterFD(virEventPtr ptr) that associates ptr with the > standard three virEvent*HandleImpl callbacks, thus allowing those > callbacks to be static to event.c rather than having multiple files have > to even be aware of how those particular callbacks are named. That > would imply making the storage for those callbacks belong to a struct, > rather than being static variables in src/util/event.c. At any rate, it > seems like there could still be a lot of beneficial refactoring done to > this code. > >> diff --git a/daemon/event.c b/src/util/event_impl.c >> similarity index 99% >> rename from daemon/event.c >> rename to src/util/event_impl.c >> index 89ca9f0..4876088 100644 >> --- a/daemon/event.c >> +++ b/src/util/event_impl.c >> @@ -32,7 +32,7 @@ >> >> #include "threads.h" >> #include "logging.h" >> -#include "event.h" >> +#include "event_impl.h" >> #include "memory.h" >> #include "util.h" >> #include "ignore-value.h" >> diff --git a/daemon/event.h b/src/util/event_impl.h > > See how compact that is? :) > >> +++ b/tests/Makefile.am >> @@ -402,7 +402,7 @@ virbuftest_LDADD = $(LDADDS) >> >> if WITH_LIBVIRTD >> eventtest_SOURCES = \ >> - eventtest.c testutils.h testutils.c ../daemon/event.c >> + eventtest.c testutils.h testutils.c >> eventtest_LDADD = -lrt $(LDADDS) >> endif > > Hmm, I wonder if this test only previously depended on WITH_LIBVIRTD > because it used ../daemon/event.c, and if that's the case, we can > probably simplify the Makefile to remove the conditionals. In fact, > that's a good idea anyways, since someone using ./autogen.sh > --without-libvirtd should still be able to use events. OK. > > Finally - this failed to build for me: > > make[2]: Entering directory `/home/remote/eblake/libvirt/daemon' > CC libvirtd-libvirtd.o > CC libvirtd-mdns.o > cc1: warnings being treated as errors > mdns.c: In function 'libvirtd_mdns_watch_dispatch': > mdns.c:233:5: error: implicit declaration of function > 'virEventHandleTypeToPollEvent' [-Wimplicit-function-declaration] It is my mistake, I only test this patch on Linux. > > so you missed at least one affected file. This appears to fix it for > me, but it's worth double-checking for anyone else that might be > conditionally built and relies on the old header location: > > diff --git i/daemon/mdns.c w/daemon/mdns.c > index ae8dc40..29a164b 100644 > --- i/daemon/mdns.c > +++ w/daemon/mdns.c > @@ -1,6 +1,7 @@ > /* > * mdns.c: advertise libvirt hypervisor connections > * > + * Copyright (C) 2010 Red Hat, Inc. > * Copyright (C) 2007 Daniel P. Berrange > * > * Derived from Avahi example service provider code. > @@ -39,7 +40,7 @@ > > #include "libvirtd.h" > #include "mdns.h" > -#include "event.h" > +#include "event_impl.h" > #include "memory.h" > > #define AVAHI_DEBUG(fmt, ...) DEBUG(fmt, __VA_ARGS__) > > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
