On 4/23/2012 11:42 AM, Angus Salkeld wrote:
Hi all
Fabio noticed that we have some odd attributes on the qb_ipc_header structs.
They currently have (aligned(8)) and should really have (packed) for efficient packing on-wire (http://gcc.gnu.org/onlinedocs/gcc-3.1/gcc/Type-Attributes.html).
He noticed this while doing some valgind testing.
This is not so easy to fix as it will break ABI.
So like this:
diff --git a/include/qb/qbipc_common.h b/include/qb/qbipc_common.h index 0a4118c..7af2c7e 100644 --- a/include/qb/qbipc_common.h +++ b/include/qb/qbipc_common.h @@ -32,15 +32,15 @@ extern "C" { /* *INDENT-ON* */
struct qb_ipc_request_header {
int32_t id __attribute__ ((aligned(8)));
int32_t size __attribute__ ((aligned(8)));
-} __attribute__ ((aligned(8)));
int32_t id;
int32_t size;
+} __attribute__ (packed);
struct qb_ipc_response_header {
int32_t id __attribute__ ((aligned(8)));
int32_t size __attribute__ ((aligned(8)));
int32_t error __attribute__ ((aligned(8)));
-} __attribute__ ((aligned(8)));
int32_t id;
int32_t size;
int32_t error;
+} __attribute__ (packed);
enum qb_ipc_type { QB_IPC_SOCKET,
Because the alignment changes per arch, this is not so easy to work around.
Is this a problem? Can we just leave it as-is? What other options are there?
One step back here..
The issue here is that we get one error/warning for each packet we send on-wire from valgrind for uninitialized memory passing around, this is because the aligned(8) for each structure entry, adds padding.
For operational / runtime / production, this has only the issue to double the size of the structure itself, leaving less space for user data (aka a small per hit).
But for debugging purposes this is a nightmare and makes valgrind very hard to use.
We have different options here, but only correct one, as Angus already mention, is going to break ABI and onwire compat.
The other possible solutions are:
1 memset (&qb_ipc_*header,..)
2 silence valgrind (http://valgrind.org/docs/manual/manual-core.html#manual-core.suppress)
3 rewrite the structure to add explicit padding instead of aligned and init those values from corosync (this isn´t so much better than adding memset, but it makes it explicit what´s in the structure and in theory we gain 2 extra fields for further use, without breaking onwire).
If something had to happen, i´d personally vote for 3. It´s a bit time consuming to go around looking for all header init, but better than being unable to use valgrind at runtime.
Angus, there are also other bits and pieces that use aligned in libqb, might be worth checking those too.
Fabio