On 23/04/12 12:05 +0200, Fabio M. Di Nitto wrote:
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).
On all arches - are you sure of the padding size and whether the padding
needs to be before or after the value?
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.
I think the logging one is correct (qblog.h).
There are some in ipc_int.h but they are internal so easiser to change
and only used for connection setup - so less important (as far as
wasting space).
-Angus
Fabio
_______________________________________________
quarterback-devel mailing list
quarterback-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/quarterback-devel