[libvirt] [PATCH] fix libvirt alignment on arm platforms
With the changes added by the latest commits (e.g. 8a29ffcf9aee45b61a0a12ee45c656cfd52333e8) related to "new events feature v2", we are unable to compile libvirt on ARM target (OMAP5). The error is due to alignment: conf/domain_event.c: In function 'virDomainEventDispatchDefaultFunc': conf/domain_event.c:1198:30: error: cast increases required alignment of target type [-Werror=cast-align] conf/domain_event.c:1314:34: error: cast increases required alignment of target type [-Werror=cast-align] cc1: all warnings being treated as errors Using ATTRIBUTE_PACKED we force a structure to not follow architecture and compiler best alignments. --- src/conf/domain_event.c | 20 ++++++++++---------- src/conf/network_event.c | 2 +- src/conf/object_event_private.h | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 64035f7..7d58367 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -76,7 +76,7 @@ struct _virDomainEventLifecycle { int type; int detail; -}; +} ATTRIBUTE_PACKED; typedef struct _virDomainEventLifecycle virDomainEventLifecycle; typedef virDomainEventLifecycle *virDomainEventLifecyclePtr; @@ -84,7 +84,7 @@ struct _virDomainEventRTCChange { virDomainEvent parent; long long offset; -}; +} ATTRIBUTE_PACKED; typedef struct _virDomainEventRTCChange virDomainEventRTCChange; typedef virDomainEventRTCChange *virDomainEventRTCChangePtr; @@ -92,7 +92,7 @@ struct _virDomainEventWatchdog { virDomainEvent parent; int action; -}; +} ATTRIBUTE_PACKED; typedef struct _virDomainEventWatchdog virDomainEventWatchdog; typedef virDomainEventWatchdog *virDomainEventWatchdogPtr; @@ -103,7 +103,7 @@ struct _virDomainEventIOError { char *devAlias; int action; char *reason; -}; +} ATTRIBUTE_PACKED; typedef struct _virDomainEventIOError virDomainEventIOError; typedef virDomainEventIOError *virDomainEventIOErrorPtr; @@ -113,7 +113,7 @@ struct _virDomainEventBlockJob { char *path; int type; int status; -}; +} ATTRIBUTE_PACKED; typedef struct _virDomainEventBlockJob virDomainEventBlockJob; typedef virDomainEventBlockJob *virDomainEventBlockJobPtr; @@ -125,7 +125,7 @@ struct _virDomainEventGraphics { virDomainEventGraphicsAddressPtr remote; char *authScheme; virDomainEventGraphicsSubjectPtr subject; -}; +} ATTRIBUTE_PACKED; typedef struct _virDomainEventGraphics virDomainEventGraphics; typedef virDomainEventGraphics *virDomainEventGraphicsPtr; @@ -136,7 +136,7 @@ struct _virDomainEventDiskChange { char *newSrcPath; char *devAlias; int reason; -}; +} ATTRIBUTE_PACKED; typedef struct _virDomainEventDiskChange virDomainEventDiskChange; typedef virDomainEventDiskChange *virDomainEventDiskChangePtr; @@ -145,7 +145,7 @@ struct _virDomainEventTrayChange { char *devAlias; int reason; -}; +} ATTRIBUTE_PACKED; typedef struct _virDomainEventTrayChange virDomainEventTrayChange; typedef virDomainEventTrayChange *virDomainEventTrayChangePtr; @@ -154,7 +154,7 @@ struct _virDomainEventBalloonChange { /* In unit of 1024 bytes */ unsigned long long actual; -}; +} ATTRIBUTE_PACKED; typedef struct _virDomainEventBalloonChange virDomainEventBalloonChange; typedef virDomainEventBalloonChange *virDomainEventBalloonChangePtr; @@ -162,7 +162,7 @@ struct _virDomainEventDeviceRemoved { virDomainEvent parent; char *devAlias; -}; +} ATTRIBUTE_PACKED; typedef struct _virDomainEventDeviceRemoved virDomainEventDeviceRemoved; typedef virDomainEventDeviceRemoved *virDomainEventDeviceRemovedPtr; diff --git a/src/conf/network_event.c b/src/conf/network_event.c index 4a02523..1ffcf6c 100644 --- a/src/conf/network_event.c +++ b/src/conf/network_event.c @@ -32,7 +32,7 @@ struct _virNetworkEventLifecycle { virObjectEvent parent; int type; -}; +} ATTRIBUTE_PACKED; typedef struct _virNetworkEventLifecycle virNetworkEventLifecycle; typedef virNetworkEventLifecycle *virNetworkEventLifecyclePtr; diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h index f41f432..34cd902 100644 --- a/src/conf/object_event_private.h +++ b/src/conf/object_event_private.h @@ -73,7 +73,7 @@ struct _virObjectEvent { virObject parent; int eventID; virObjectMeta meta; -}; +}ATTRIBUTE_PACKED; virClassPtr virClassForObjectEvent(void); -- 1.7.9.5
On 01.01.1970 01:03, Michele Paolino wrote:
With the changes added by the latest commits (e.g. 8a29ffcf9aee45b61a0a12ee45c656cfd52333e8) related to "new events feature v2", we are unable to compile libvirt on ARM target (OMAP5). The error is due to alignment: conf/domain_event.c: In function 'virDomainEventDispatchDefaultFunc': conf/domain_event.c:1198:30: error: cast increases required alignment of target type [-Werror=cast-align] conf/domain_event.c:1314:34: error: cast increases required alignment of target type [-Werror=cast-align] cc1: all warnings being treated as errors
Using ATTRIBUTE_PACKED we force a structure to not follow architecture and compiler best alignments. --- src/conf/domain_event.c | 20 ++++++++++---------- src/conf/network_event.c | 2 +- src/conf/object_event_private.h | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 64035f7..7d58367 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -76,7 +76,7 @@ struct _virDomainEventLifecycle {
int type; int detail; -}; +} ATTRIBUTE_PACKED;
(What an ancient e-mail :-) ) I've raised the problem here: https://www.redhat.com/archives/libvir-list/2013-December/msg00635.html And Eric replied suggesting a fix: https://www.redhat.com/archives/libvir-list/2013-December/msg00662.html But I must say I like your approach more. Eric? Michal
On 12/12/2013 11:55 AM, Michal Privoznik wrote:
int detail; -}; +} ATTRIBUTE_PACKED;
(What an ancient e-mail :-) )
1970 was how many years before libvirt was even started?
I've raised the problem here:
https://www.redhat.com/archives/libvir-list/2013-December/msg00635.html
And Eric replied suggesting a fix:
https://www.redhat.com/archives/libvir-list/2013-December/msg00662.html
But I must say I like your approach more. Eric?
I still think ATTRIBUTE_PACKED in a parent class is wrong; it forces the children to be packed, and may make it impossible to implement atomic operations on a child member that was supposed to be aligned. I'd much rather fix the alignment in the parent class using portable constructs than a compiler-specific non-alignment directive. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12.12.2013 20:27, Eric Blake wrote:
On 12/12/2013 11:55 AM, Michal Privoznik wrote:
int detail; -}; +} ATTRIBUTE_PACKED;
(What an ancient e-mail :-) )
1970 was how many years before libvirt was even started?
I've raised the problem here:
https://www.redhat.com/archives/libvir-list/2013-December/msg00635.html
And Eric replied suggesting a fix:
https://www.redhat.com/archives/libvir-list/2013-December/msg00662.html
But I must say I like your approach more. Eric?
I still think ATTRIBUTE_PACKED in a parent class is wrong; it forces the children to be packed, and may make it impossible to implement atomic operations on a child member that was supposed to be aligned. I'd much rather fix the alignment in the parent class using portable constructs than a compiler-specific non-alignment directive.
Oh right. The locking could be impossible, because if two struct members are packed in a word, e.g.: struct S { bool a; bool b; ... }; then modifying S.a requires whole struct to be fetched from mem, modifying S.a a and storing it back. However, even if our locking code to protect say S.b was written well, S.b can still get changed without lock. As S.a is being changed, another thread changes S.b, but storing S.a means overwriting S.b without lock held. I have not thought of that. So NACK then - we need Eric's approach. Michal
On 13/12/2013 08:54, Michal Privoznik wrote:
On 12.12.2013 20:27, Eric Blake wrote:
On 12/12/2013 11:55 AM, Michal Privoznik wrote:
int detail; -}; +} ATTRIBUTE_PACKED;
(What an ancient e-mail :-) ) 1970 was how many years before libvirt was even started? Indeed, this is a 43 years old thread :-)
I've had some problem with the ntpd deamon on the OMAP5 yesterday. For this reason I've resent the patch with the prefix [resend].
I've raised the problem here:
https://www.redhat.com/archives/libvir-list/2013-December/msg00635.html
And Eric replied suggesting a fix:
https://www.redhat.com/archives/libvir-list/2013-December/msg00662.html
But I must say I like your approach more. Eric? I still think ATTRIBUTE_PACKED in a parent class is wrong; it forces the children to be packed, and may make it impossible to implement atomic operations on a child member that was supposed to be aligned. I'd much rather fix the alignment in the parent class using portable constructs than a compiler-specific non-alignment directive.
Oh right. The locking could be impossible, because if two struct members are packed in a word, e.g.:
struct S { bool a; bool b; ... };
then modifying S.a requires whole struct to be fetched from mem, modifying S.a a and storing it back. However, even if our locking code to protect say S.b was written well, S.b can still get changed without lock. As S.a is being changed, another thread changes S.b, but storing S.a means overwriting S.b without lock held.
I have not thought of that. So NACK then - we need Eric's approach.
I saw the Eric's patch. I will test it today.
Michal
-- *Michele Paolino*, Virtualization R&D Engineer Virtual Open Systems /Open Source KVM Virtualization Developments/ /Multicore Systems Virtualization Porting Services/ Web/:/www.virtualopensystems. <http://www.virtualopensystems.com/>com <http://www.virtualopensystems.com/>
participants (3)
-
Eric Blake -
Michal Privoznik -
Michele Paolino