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

[Eric Blake]

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 <wency at 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   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101223/079039a3/attachment-0001.sig>