Re: [libvirt] [PATCH 00/34] network events feature v2
On 12/12/2013 09:05 AM, Michal Privoznik wrote:
>> Or maybe even change _virObject to contain a union:
>>
>> struct _virObject {
>> union {
>> long long align;
>> struct {
>> unsigned int magic;
>> int refs;
>> } s;
>> } u;
>> virClassPtr klass;
>> }
>
> Yep. I can confirm that this works. This and all the subsequent code
> adaptations made me able to compile again. Will you post it as a patch
> please?
Yes, I'll turn it into a formal patch.
--
Eric Blake eblake redhat com+1-919-301-3266
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
Re: [libvirt] [PATCH 00/34] network events feature v2
Sorry, I noticed just now that here you are discussing about the problem
with the compilation of libvirt on ARM platforms. An alternative
solution can be ATTRIBUTE_PACKED.
I've sent right now to the ML a patch based on this.
Michele
On 12/12/2013 17:11, Daniel P. Berrange wrote:
On Wed, Dec 11, 2013 at 12:28:32PM -0700, Eric Blake wrote:
On 12/11/2013 12:15 PM, Eric Blake wrote:
struct _virObjectEvent {
virObject parent;
int eventID;
virObjectMeta meta;
};
Only has alignment specified by virObject (which in turn is unsigned
int, int, void*),
struct _virObject {
unsigned int magic;
int refs;
virClassPtr klass;
};
I think one possible solution would be as simple as altering
src/util/virobject.h to change 'magic' from 'unsigned int' to 'unsigned
long long' - then ALL virObject structs will be forcefully aligned to
the worst case between void* and long long, so that any subclass can use
long long without requiring stricter alignment than the parent class,
and so that downcasting code like domain_event.c no longer warns. But
it does make every object consume more memory on 64-bit platforms (from
16 bytes into 24 bytes), is that okay?
Or maybe even change _virObject to contain a union:
struct _virObject {
union {
long long align;
struct {
unsigned int magic;
int refs;
} s;
} u;
virClassPtr klass;
}
which keeps the size at 16 bytes on 64-bit platform, keeps things at 12
bytes on 32-bit platforms that don't care about long long alignment, and
for ARM (*) would change things from 12 to 16 bytes with 8-byte
alignment for the long long.
Yeah, that means using obj->u.s.refs instead of obj->refs, but most code
shouldn't be directly mucking with object-internal fields, so hopefully
the fallout would be limited to just virobject.c (if only C99 had
anonymous struct/unions; C11 does, but we don't require that yet).
(*) Am I correct that your platform with the compile failure is a 32-bit
ARM platform with 4-byte pointers? Because if it were 64-bit, then I
would have guessed that an 8-byte pointer would already be forcing
8-byte alignment, such that use of 'long long' in a subclass wouldn't be
warning about changed alignment.
That seems reasonable to me - it makes sense that we should have
our base object type be nicely aligned, instead of trying to fix
this in the events code (and potentially anywhere else using
objects in the future).
Daniel
--
*Michele Paolino*, Virtualization R&D Engineer
Virtual Open Systems
/Open Source KVM Virtualization Developments/
/Multicore Systems Virtualization Porting Services/
Web/:/www.virtualopensystems.
Re: [libvirt] [PATCH 00/34] network events feature v2
On Wed, Dec 11, 2013 at 12:28:32PM -0700, Eric Blake wrote:
> On 12/11/2013 12:15 PM, Eric Blake wrote:
>
> > struct _virObjectEvent {
> > virObject parent;
> > int eventID;
> > virObjectMeta meta;
> > };
> >
> > Only has alignment specified by virObject (which in turn is unsigned
> > int, int, void*),
>
> struct _virObject {
> unsigned int magic;
> int refs;
> virClassPtr klass;
> };
>
>
> > I think one possible solution would be as simple as altering
> > src/util/virobject.h to change 'magic' from 'unsigned int' to 'unsigned
> > long long' - then ALL virObject structs will be forcefully aligned to
> > the worst case between void* and long long, so that any subclass can use
> > long long without requiring stricter alignment than the parent class,
> > and so that downcasting code like domain_event.c no longer warns. But
> > it does make every object consume more memory on 64-bit platforms (from
> > 16 bytes into 24 bytes), is that okay?
>
> Or maybe even change _virObject to contain a union:
>
> struct _virObject {
> union {
> long long align;
> struct {
> unsigned int magic;
> int refs;
> } s;
> } u;
> virClassPtr klass;
> }
>
> which keeps the size at 16 bytes on 64-bit platform, keeps things at 12
> bytes on 32-bit platforms that don't care about long long alignment, and
> for ARM (*) would change things from 12 to 16 bytes with 8-byte
> alignment for the long long.
>
> Yeah, that means using obj->u.s.refs instead of obj->refs, but most code
> shouldn't be directly mucking with object-internal fields, so hopefully
> the fallout would be limited to just virobject.c (if only C99 had
> anonymous struct/unions; C11 does, but we don't require that yet).
>
> (*) Am I correct that your platform with the compile failure is a 32-bit
> ARM platform with 4-byte pointers? Because if it were 64-bit, then I
> would have guessed that an 8-byte pointer would already be forcing
> 8-byte alignment, such that use of 'long long' in a subclass wouldn't be
> warning about changed alignment.
That seems reasonable to me - it makes sense that we should have
our base object type be nicely aligned, instead of trying to fix
this in the events code (and potentially anywhere else using
objects in the future).
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/34] network events feature v2
On 11.12.2013 20:28, Eric Blake wrote:
> On 12/11/2013 12:15 PM, Eric Blake wrote:
>
>> struct _virObjectEvent {
>> virObject parent;
>> int eventID;
>> virObjectMeta meta;
>> };
>>
>> Only has alignment specified by virObject (which in turn is unsigned
>> int, int, void*),
>
> struct _virObject {
> unsigned int magic;
> int refs;
> virClassPtr klass;
> };
>
>
>> I think one possible solution would be as simple as altering
>> src/util/virobject.h to change 'magic' from 'unsigned int' to 'unsigned
>> long long' - then ALL virObject structs will be forcefully aligned to
>> the worst case between void* and long long, so that any subclass can use
>> long long without requiring stricter alignment than the parent class,
>> and so that downcasting code like domain_event.c no longer warns. But
>> it does make every object consume more memory on 64-bit platforms (from
>> 16 bytes into 24 bytes), is that okay?
>
> Or maybe even change _virObject to contain a union:
>
> struct _virObject {
> union {
> long long align;
> struct {
> unsigned int magic;
> int refs;
> } s;
> } u;
> virClassPtr klass;
> }
Yep. I can confirm that this works. This and all the subsequent code
adaptations made me able to compile again. Will you post it as a patch
please?
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/34] network events feature v2
On 12/11/2013 12:15 PM, Eric Blake wrote:
> struct _virObjectEvent {
> virObject parent;
> int eventID;
> virObjectMeta meta;
> };
>
> Only has alignment specified by virObject (which in turn is unsigned
> int, int, void*),
struct _virObject {
unsigned int magic;
int refs;
virClassPtr klass;
};
> I think one possible solution would be as simple as altering
> src/util/virobject.h to change 'magic' from 'unsigned int' to 'unsigned
> long long' - then ALL virObject structs will be forcefully aligned to
> the worst case between void* and long long, so that any subclass can use
> long long without requiring stricter alignment than the parent class,
> and so that downcasting code like domain_event.c no longer warns. But
> it does make every object consume more memory on 64-bit platforms (from
> 16 bytes into 24 bytes), is that okay?
Or maybe even change _virObject to contain a union:
struct _virObject {
union {
long long align;
struct {
unsigned int magic;
int refs;
} s;
} u;
virClassPtr klass;
}
which keeps the size at 16 bytes on 64-bit platform, keeps things at 12
bytes on 32-bit platforms that don't care about long long alignment, and
for ARM (*) would change things from 12 to 16 bytes with 8-byte
alignment for the long long.
Yeah, that means using obj->u.s.refs instead of obj->refs, but most code
shouldn't be directly mucking with object-internal fields, so hopefully
the fallout would be limited to just virobject.c (if only C99 had
anonymous struct/unions; C11 does, but we don't require that yet).
(*) Am I correct that your platform with the compile failure is a 32-bit
ARM platform with 4-byte pointers? Because if it were 64-bit, then I
would have guessed that an 8-byte pointer would already be forcing
8-byte alignment, such that use of 'long long' in a subclass wouldn't be
warning about changed alignment.
--
Eric Blake eblake redhat com+1-919-301-3266
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
Re: [libvirt] [PATCH 00/34] network events feature v2
On 12/11/2013 06:18 AM, Michal Privoznik wrote:
>
> I'm getting some compile errors on ARM:
>
> CC conf/libvirt_conf_la-domain_event.lo
> conf/domain_event.c: In function 'virDomainEventDispatchDefaultFunc':
> conf/domain_event.c:1148:30: error: cast increases required alignment of
> target type [-Werror=cast-align]
> rtcChangeEvent = (virDomainEventRTCChangePtr)event;
Hmm. In this case, event is virObjectEventPtr, and rtcChangeEvent is
virDomainEventRTCChangePtr. Comparing the two structs, I see the problem:
struct _virObjectEvent {
virObject parent;
int eventID;
virObjectMeta meta;
};
Only has alignment specified by virObject (which in turn is unsigned
int, int, void*), int, and virObjectMeta (which in turn is int, char*,
unsigned char[]).
vs.
struct _virDomainEventRTCChange {
virDomainEvent parent;
long long offset;
};
I think one possible solution would be as simple as altering
src/util/virobject.h to change 'magic' from 'unsigned int' to 'unsigned
long long' - then ALL virObject structs will be forcefully aligned to
the worst case between void* and long long, so that any subclass can use
long long without requiring stricter alignment than the parent class,
and so that downcasting code like domain_event.c no longer warns. But
it does make every object consume more memory on 64-bit platforms (from
16 bytes into 24 bytes), is that okay?
--
Eric Blake eblake redhat com+1-919-301-3266
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
Re: [libvirt] [PATCH 00/34] network events feature v2
On 29.11.2013 16:18, Cédric Bosdonnat wrote: > This patch serie is replacing the previous one I sent. The improvements that > were made in between are: > * splitting the 2 huge commits into smaller reviewable ones. > * Get rid of the huge union in virDomainEvent and use virObjects instead. > * No domain events-related code in object_event.c. I left the network events > related code there as it would be pretty small in a separate set of files. > * Rebased on the python split repository > > I saw something weird in the domain events code when working on this: > VIR_DOMAIN_EVENT_LAST is defined if VIR_ENUM_SENTINELS is defined, but is > generally used independently of it. I did the same for the > VIR_NETWORK_EVENT_LAST > thought I'm not sure that's the correct thing to do. > > These changes are all about bringing events for network object like the > ones existing for domains. This feature is needed for virt-manager to > refresh its UI when networks are started/destroyed/defined/undefined. > > The network events are implemented in the test, bridge network and remote > drivers ATM. > > Cédric Bosdonnat (34): > Added domain start/stop/define/undefine event unit tests > Rename virDomainEventCallback to virObjectEventCallback > Renamed virDomainMeta to virObjectMeta > Renamed virDomainEventQueue to virObjectEventQueue > Renamed virDomainEventState to virObjectEventState > Renamed virDomainEventCallbackList* to virObjectEventCallbackList* > Created virObjectEventStateRegisterID > virObject-ified virDomainEvent > Create virDomainEventLifecycle to start removing the huge union > Renamed virDomainEventNew* to virDomainEventLifecycleNew* > Renamed virDomainEventNewInternal to virDomainEventNew > Create virDomainEventRTCChange to get rid of the huge union > Created virDomainEventWatchdog to get rid of the huge union > Created virDomainEventIOError > Created virDomainEventGraphics > Created virDomainEventBlockJob > Create virDomainEventDiskChange > Created virDomainEventTrayChange > Created virDomainEventBalloonChange > Created virDomainEventDeviceRemoved and removed the huge union > Changed the remaining domain event creation methods results to void* > Removed virDomainEventPtr in favor of virObjectEventPtr > Add object event namespaces for the event IDs > Renamed virDomainEventTimer to virObjectEventTimer > Split the virObjectEvent and virDomainEvent* to separate them after > Extracted common parts of domain_event.[ch] to object_event.[ch] > Added virNetworkEventLifecycle object > Added API for network events similar to the one from Domain events > test driver: renamed testDomainEventQueue into testObjectEventQueue > test driver: implemented network events > Add network events unit tests > daemon/remote.c: renamed remoteDispatchDomainEventSend > Add network events to the remote driver > Added network events to the bridged network driver > > .gitignore |1 + > cfg.mk |6 +- > daemon/libvirtd.h|1 + > daemon/remote.c | 175 ++- > include/libvirt/libvirt.h.in | 86 ++ > src/Makefile.am |6 + > src/conf/domain_event.c | 1954 > ++ > src/conf/domain_event.h | 219 ++-- > src/conf/object_event.c | 903 > src/conf/object_event.h | 113 ++ > src/conf/object_event_private.h | 113 ++ > src/driver.h | 14 + > src/libvirt.c| 133 +++ > src/libvirt_private.syms | 25 +- > src/libvirt_public.syms |7 + > src/libxl/libxl_conf.h |2 +- > src/libxl/libxl_driver.c | 46 +- > src/lxc/lxc_conf.h |2 +- > src/lxc/lxc_driver.c | 54 +- > src/lxc/lxc_process.c| 20 +- > src/network/bridge_driver.c | 89 ++ > src/network/bridge_driver_platform.h |3 + > src/parallels/parallels_utils.h |2 +- > src/qemu/qemu_conf.h |2 +- > src/qemu/qemu_domain.c |6 +- > src/qemu/qemu_domain.h |2 +- > src/qemu/qemu_driver.c | 116 +- > src/qemu/qemu_hotplug.c | 10 +- > src/qemu/qemu_migration.c| 38 +- > src/qemu/qemu_process.c | 70 +- > src/remote/remote_driver.c | 178 +++- > src/remote/remote_protocol.x | 46 +- > src/test/test_driver.c | 197 ++-- > src/uml/uml_conf.h |2 +- > src/uml/uml_driver.c | 44 +- > src/vbox/vbox_tmpl.c | 22 +- > src/xen/xen_driver.c | 10 +- > src/xen/xen_driver.h |4 +- > src/xen/xen_inotify.c| 10 +- > src/xen/xs_internal.c
Re: [libvirt] [PATCH 00/34] network events feature v2
On Fri, Nov 29, 2013 at 04:18:36PM +0100, Cédric Bosdonnat wrote: > This patch serie is replacing the previous one I sent. The improvements that > were made in between are: > * splitting the 2 huge commits into smaller reviewable ones. > * Get rid of the huge union in virDomainEvent and use virObjects instead. > * No domain events-related code in object_event.c. I left the network events > related code there as it would be pretty small in a separate set of files. > * Rebased on the python split repository This all looks pretty good to me. The only thing I'm not really liking is the VIR_EVENT_NAMESPACE stuff exposed in the public header. Since we have separate public APIs for registering event callbacks for each type of object, there is no need to have namespaces for event IDs from an app developers POV. I understand you want to encode things into a single event ID space internally for ease of handling, but I think we should not expose this to apps. > > I saw something weird in the domain events code when working on this: > VIR_DOMAIN_EVENT_LAST is defined if VIR_ENUM_SENTINELS is defined, but is > generally used independently of it. I did the same for the > VIR_NETWORK_EVENT_LAST > thought I'm not sure that's the correct thing to do. The internal code alwas has VIR_ENUM_SENTINELS defined so can rely on using any _LAST constant. We simply hide from the client applications by default. > These changes are all about bringing events for network object like the > ones existing for domains. This feature is needed for virt-manager to > refresh its UI when networks are started/destroyed/defined/undefined. > > The network events are implemented in the test, bridge network and remote > drivers ATM. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 00/34] network events feature v2
This patch serie is replacing the previous one I sent. The improvements that were made in between are: * splitting the 2 huge commits into smaller reviewable ones. * Get rid of the huge union in virDomainEvent and use virObjects instead. * No domain events-related code in object_event.c. I left the network events related code there as it would be pretty small in a separate set of files. * Rebased on the python split repository I saw something weird in the domain events code when working on this: VIR_DOMAIN_EVENT_LAST is defined if VIR_ENUM_SENTINELS is defined, but is generally used independently of it. I did the same for the VIR_NETWORK_EVENT_LAST thought I'm not sure that's the correct thing to do. These changes are all about bringing events for network object like the ones existing for domains. This feature is needed for virt-manager to refresh its UI when networks are started/destroyed/defined/undefined. The network events are implemented in the test, bridge network and remote drivers ATM. Cédric Bosdonnat (34): Added domain start/stop/define/undefine event unit tests Rename virDomainEventCallback to virObjectEventCallback Renamed virDomainMeta to virObjectMeta Renamed virDomainEventQueue to virObjectEventQueue Renamed virDomainEventState to virObjectEventState Renamed virDomainEventCallbackList* to virObjectEventCallbackList* Created virObjectEventStateRegisterID virObject-ified virDomainEvent Create virDomainEventLifecycle to start removing the huge union Renamed virDomainEventNew* to virDomainEventLifecycleNew* Renamed virDomainEventNewInternal to virDomainEventNew Create virDomainEventRTCChange to get rid of the huge union Created virDomainEventWatchdog to get rid of the huge union Created virDomainEventIOError Created virDomainEventGraphics Created virDomainEventBlockJob Create virDomainEventDiskChange Created virDomainEventTrayChange Created virDomainEventBalloonChange Created virDomainEventDeviceRemoved and removed the huge union Changed the remaining domain event creation methods results to void* Removed virDomainEventPtr in favor of virObjectEventPtr Add object event namespaces for the event IDs Renamed virDomainEventTimer to virObjectEventTimer Split the virObjectEvent and virDomainEvent* to separate them after Extracted common parts of domain_event.[ch] to object_event.[ch] Added virNetworkEventLifecycle object Added API for network events similar to the one from Domain events test driver: renamed testDomainEventQueue into testObjectEventQueue test driver: implemented network events Add network events unit tests daemon/remote.c: renamed remoteDispatchDomainEventSend Add network events to the remote driver Added network events to the bridged network driver .gitignore |1 + cfg.mk |6 +- daemon/libvirtd.h|1 + daemon/remote.c | 175 ++- include/libvirt/libvirt.h.in | 86 ++ src/Makefile.am |6 + src/conf/domain_event.c | 1954 ++ src/conf/domain_event.h | 219 ++-- src/conf/object_event.c | 903 src/conf/object_event.h | 113 ++ src/conf/object_event_private.h | 113 ++ src/driver.h | 14 + src/libvirt.c| 133 +++ src/libvirt_private.syms | 25 +- src/libvirt_public.syms |7 + src/libxl/libxl_conf.h |2 +- src/libxl/libxl_driver.c | 46 +- src/lxc/lxc_conf.h |2 +- src/lxc/lxc_driver.c | 54 +- src/lxc/lxc_process.c| 20 +- src/network/bridge_driver.c | 89 ++ src/network/bridge_driver_platform.h |3 + src/parallels/parallels_utils.h |2 +- src/qemu/qemu_conf.h |2 +- src/qemu/qemu_domain.c |6 +- src/qemu/qemu_domain.h |2 +- src/qemu/qemu_driver.c | 116 +- src/qemu/qemu_hotplug.c | 10 +- src/qemu/qemu_migration.c| 38 +- src/qemu/qemu_process.c | 70 +- src/remote/remote_driver.c | 178 +++- src/remote/remote_protocol.x | 46 +- src/test/test_driver.c | 197 ++-- src/uml/uml_conf.h |2 +- src/uml/uml_driver.c | 44 +- src/vbox/vbox_tmpl.c | 22 +- src/xen/xen_driver.c | 10 +- src/xen/xen_driver.h |4 +- src/xen/xen_inotify.c| 10 +- src/xen/xs_internal.c| 20 +- tests/Makefile.am|7 + tests/objecteventtest.c | 407 +++ tests/qemuhotplugtest.c |2 +- 43 files changed, 3525 insertions(+), 1642 deletions(-) create mode 100644 s
