On Thu, Jul 7, 2016 at 9:05 AM, Mike Holmes <[email protected]> wrote:
> If it can't be done in a backwards compatible way this has to go to > TigerMoth. > > Do we patch in Monarch and fix properly there ? > This should be fixed in Monarch. I'd prefer to fix this without reverting Petri's header restructure patch series, but that's what introduced this problem. > > Mike > > On 7 July 2016 at 09:48, Bill Fischofer <[email protected]> wrote: > >> On Thu, Jul 7, 2016 at 8:38 AM, Savolainen, Petri (Nokia - FI/Espoo) < >> [email protected]> wrote: >> >> > >> > >> > From: Bill Fischofer [mailto:[email protected]] >> > Sent: Thursday, July 07, 2016 3:01 PM >> > To: Savolainen, Petri (Nokia - FI/Espoo) < >> > [email protected]> >> > Cc: [email protected] >> > Subject: Re: [lng-odp] [PATCH] linux-generic: byteorder: avoid bitfield >> > order doxygen omissions >> > >> > >> > >> > On Thu, Jul 7, 2016 at 6:57 AM, Savolainen, Petri (Nokia - FI/Espoo) < >> > [email protected]> wrote: >> > >> > >> > From: Bill Fischofer [mailto:[email protected]] >> > Sent: Thursday, July 07, 2016 2:42 PM >> > To: Savolainen, Petri (Nokia - FI/Espoo) < >> > [email protected]> >> > Cc: [email protected] >> > Subject: Re: [lng-odp] [PATCH] linux-generic: byteorder: avoid bitfield >> > order doxygen omissions >> > >> > >> > On Thu, Jul 7, 2016 at 2:46 AM, Savolainen, Petri (Nokia - FI/Espoo) < >> > [email protected]> wrote: >> > I'm OK with the change, but didn't do it as part of my patch set since >> > it's an API change. I think the change is not a big deal and should be >> done >> > to align the usage of byte/bitfield defines. The patch should be tagged >> > with api: prefix and go through api-next. >> > >> > Also, API spec could be more explicit about the byte/bitfield defines >> and >> > how application should use/test those. I think bitfield defines should >> work >> > the same way as the current byte order defines. >> > >> > #if (ODP_BYTE_ORDER == ODP_BIG_ENDIAN) >> > // big endian byte order >> > #else >> > // little endian byte order >> > #endif >> > >> > >> > -Petri >> > >> > Not making an API change was the reason I did this change this way. This >> > is now an implementation change only. The change that would involve an >> API >> > change would have been to define an ODP_BITFIELD_ORDER variable to >> mirror >> > the ODP_BYTE_ORDER variable and set that to either >> > ODP_LITTLE_ENDIAN_BITFIELD or ODP_BIG_ENDIAN_BITFIELD as needed. >> > >> > >> > > --- a/platform/linux-generic/include/protocols/tcp.h >> > > +++ b/platform/linux-generic/include/protocols/tcp.h >> > > @@ -34,7 +34,7 @@ typedef struct ODP_PACKED { >> > > odp_u32be_t ack_no; /**< Acknowledgment number */ >> > > union { >> > > odp_u16be_t doffset_flags; >> > > -#if defined(ODP_BIG_ENDIAN_BITFIELD) >> > > +#if ODP_BIG_ENDIAN_BITFIELD >> > >> > >> > The change is non-backward, API change for application code. Tcp helper >> is >> > also application code. >> > >> > Today, application may do this half of the time ... >> > >> > #if defined(ODP_BIG_ENDIAN_BITFIELD) >> > // foo >> > #else >> > // bar >> > #endif >> > >> > ... and this the other half ... >> > >> > #if defined(ODP_LITTLE_ENDIAN_BITFIELD) >> > // foo >> > #else >> > // bar >> > #endif >> > >> > >> > After the change, application would break since both >> > ODP_LITTLE_ENDIAN_BITFIELD and ODP_BIG_ENDIAN_BITFIELD are defined >> always. >> > >> > Yes, which is why the patch includes an update to both copies of tcp.h, >> > which are currently the only files that reference those #defines. >> > >> > The issue is that the recent change seems to have broken doxygen, so the >> > question is what's the best way to fix that? >> > >> > >> > << Reply to HTML mail>> >> > You must consider helpers as part of application code. There may be >> other >> > application code out there which does exactly the same thing (the #if >> > defined example above). This change would break *any* application code >> that >> > uses the defines. >> > >> >> So short of reverting your latest header restructure patch, how would you >> suggest fixing this? >> >> >> > >> > -Petri >> > >> > >> > >> > >> > >> > >> > >> > >> > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows" > > >
