On Tuesday 18 December 2018 17:50:52 Luiz Augusto von Dentz wrote:
> On Tue, 18 Dec 2018, 11:54 Pali Rohár <pali.rohar@xxxxxxxxx wrote:
>
> > Hello,
> >
> > I spotted that bluez has file profiles/audio/a2dp-codecs.h in its git
> > repository with some #if __BYTE_ORDER == checks, including #ifdef for
> > big endian systems.
> >
> > But this file is fully broken for big endian systems. For example, there
> > is following structure which is same for both little and big endian
> > systems:
> >
> > typedef struct {
> > uint32_t vendor_id;
> > uint16_t codec_id;
> > } __attribute__ ((packed)) a2dp_vendor_codec_t;
> >
>
> Byte order is not the same as packet order, in this case there is no
> difference between big or little endian because there is no use of bit
> fields, the packet ordering of the fields is the same regardless of the
> byte order meaning that vendor_id is transmitted before codec_id since the
> spec actually defines them in this way and it is wrong to alter this order.
> You can check how btmon decodes this and you would see it only decodes
> individual fields to host not the entire struct representing the packet.
>
> Which is valid only for little endian systems, as those definitions are
> > bound to little endian ordering -- not to host ordering.
> >
>
> The spec defines the packets in little endian not in host endian.
But in code, you work with host endian. Therefore uint32_t is in host
endian too. Data are transferred in little endian (as you wrote), so you
need to do some conversion from little endian to host endian.
Or you new gcc6 feature
__attribute__((packed, scalar_storage_order("little-endian")))
to tell that structure field is in little endian and not in host endian.
> Next there is a2dp_mpeg_t. It has different definition for little endian
> > and big endian systems.
> >
> > little endian:
> >
> > typedef struct {
> > uint8_t channel_mode:4;
> > uint8_t crc:1;
> > uint8_t layer:3;
> > uint8_t frequency:6;
> > uint8_t mpf:1;
> > uint8_t rfa:1;
> > uint16_t bitrate;
> > } __attribute__ ((packed)) a2dp_mpeg_t;
> >
> > big endian:
> >
> > typedef struct {
> > uint8_t layer:3;
> > uint8_t crc:1;
> > uint8_t channel_mode:4;
> > uint8_t rfa:1;
> > uint8_t mpf:1;
> > uint8_t frequency:6;
> > uint16_t bitrate;
> > } __attribute__ ((packed)) a2dp_mpeg_t;
> >
> > But as you can see, bitrate on big endian definition is incorrect. It
> > is still in little endian. Proper way would be to define bitrate as
> > "uint8_t bitrate[2];" or as "uint8_t bitrate_lo; uint8_t bitrate_hi".
> >
>
> Obviously one need to convert the value when receiving, or perhaps you are
> saying it makes no sense to use bit fields in that case? Anyway when
> accessing this the user must convert to host order of he intends to use the
> value.
Yes, that is the point, user needs to convert values. But they are
marked with types uint32_t and uint16_t, which are types normally used
for host endian. This just confuse people who copy+paste this file to
other projects.
So if correct way is to always convert these values, what about creating
some typedef uint16_t le_uint16_t or add comments that those values are
in little endian and not in host endian?
Or usage of __attribute__((packed, scalar_storage_order("little-endian")))?
> So basically this file a2dp-codecs.h is not compatible with big endian
> > systems, even there are "#elif __BYTE_ORDER == __BIG_ENDIAN" branches.
> >
> > If you are not going to support big endian systems, I suggest you to
> > drop all those "#elif __BYTE_ORDER == __BIG_ENDIAN" parts as people
> > think that existence of these macros means that code is supported on big
> > endian systems.
> >
> > It is better throw an error that big endian is broken as trying to
> > produce non-working binaries.
> >
> > Moreover, some people started copying this a2dp-codecs.h file to other
> > projects which just leads to copy+paste of broken code.
> >
> > --
> > Pali Rohár
> > pali.rohar@xxxxxxxxx
> >
--
Pali Rohár
pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
