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? > +++ 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. 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). 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. 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] 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__) -- Eric Blake ebl...@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
