10:44 a.m.
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
12:55 p.m.
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
1:27 p.m.
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
1:54 a.m.
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
2:44 a.m.
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/>
3997
days inactive
3998
days old
4 comments
3 participants
participants (3)
-
Eric Blake -
Michal Privoznik -
Michele Paolino