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

Fri Dec 24 07:16:39 UTC 2010

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 <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?
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__)
> 
> 