Re: [libvirt] [PATCH v4 1/3] move daemon/event.* into src/util/ directory

On 12/23/2010 01:56 AM, Wen Congyang wrote: 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: Why the rename? Oh, because src/util/event.c already exists. Can we merge those into one file, rather than adding the _impl variant? 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. See how compact that is? :) 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 eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org