[v2,25/26] batman-adv: packet.h, add some missing includes

  Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 packet.h | 5 +++++
 1 file changed, 5 insertions(+)
  

Hi Markus,

first of all you should know that I really appreciate your hard work.
Thank you very much for this!

On 26/12/14 12:41, Markus Pargmann wrote:
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  packet.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/packet.h b/packet.h
> index facd1feddd4e..5fd0d814b6de 100644
> --- a/packet.h
> +++ b/packet.h
> @@ -18,6 +18,11 @@
>  #ifndef _NET_BATMAN_ADV_PACKET_H_
>  #define _NET_BATMAN_ADV_PACKET_H_
>  
> +#ifdef __KERNEL__
> +#include <linux/bitops.h>
> +#include <uapi/linux/if_ether.h>
> +#endif /* __KERNEL__ */

Unfortunately we can't do this :-(

Patches sent to the kernel cannot contain conditional code on being in
the kernel or not (same is true for checks against a particular kernel
version - such code can't be in the kernel).

Patches sent to the kernel must assume that they are only for the kernel
(and for that particular version we are sending the patches against).

Of course this makes everything a bit trickier for us since we share
files between kernel and userspace .....

This is one of the reasons why we have compat.c/h in the batman-adv
package: you will see that in those files we had to implement all sort
of hackish/dirty tricks in order to keep the code in the other files
clean (compat.h/c are not sent to the kernel but are part of the
batman-adv out-of-the-tree package).


However, as far as I know, you should be able to simply do:

#include <linux/if_ether.h> (without the uapi subfolder)

and this should happily work for both the kernel and the userspace. Have
you tried that? I might be wrong - it's quite some time I haven't used
uapi headers.


Otherwise we might need to find a different solution - or live we what
we have now :)

Cheers,


> +
>  /**
>   * enum batadv_packettype - types for batman-adv encapsulated packets
>   * @BATADV_IV_OGM: originator messages for B.A.T.M.A.N. IV
>
  

Hi Antonio,

On Sat, Dec 27, 2014 at 03:30:53PM +0100, Antonio Quartulli wrote:
> Hi Markus,
> 
> first of all you should know that I really appreciate your hard work.
> Thank you very much for this!
> 
> On 26/12/14 12:41, Markus Pargmann wrote:
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  packet.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/packet.h b/packet.h
> > index facd1feddd4e..5fd0d814b6de 100644
> > --- a/packet.h
> > +++ b/packet.h
> > @@ -18,6 +18,11 @@
> >  #ifndef _NET_BATMAN_ADV_PACKET_H_
> >  #define _NET_BATMAN_ADV_PACKET_H_
> >  
> > +#ifdef __KERNEL__
> > +#include <linux/bitops.h>
> > +#include <uapi/linux/if_ether.h>
> > +#endif /* __KERNEL__ */
> 
> Unfortunately we can't do this :-(
> 
> Patches sent to the kernel cannot contain conditional code on being in
> the kernel or not (same is true for checks against a particular kernel
> version - such code can't be in the kernel).
> 
> Patches sent to the kernel must assume that they are only for the kernel
> (and for that particular version we are sending the patches against).

Oh, I checked by grepping through the kernel before changing the patch
like this.
	git grep "^#if.*def.*__KERNEL__" | grep -v include | wc -l
	145

So there are 145 uses of an ifdef __KERNEL__ outside of include
directories (I excluded include directories as they may be exported to
userspace). So I thought it would be ok.

> Of course this makes everything a bit trickier for us since we share
> files between kernel and userspace .....
> 
> This is one of the reasons why we have compat.c/h in the batman-adv
> package: you will see that in those files we had to implement all sort
> of hackish/dirty tricks in order to keep the code in the other files
> clean (compat.h/c are not sent to the kernel but are part of the
> batman-adv out-of-the-tree package).

Yes I saw that. I still don't really understand why this whole out of
tree package is necessary. But I am wondering if it wouldn't be easier
to develope and build directly in the kernel tree.

> 
> However, as far as I know, you should be able to simply do:
> 
> #include <linux/if_ether.h> (without the uapi subfolder)
> 
> and this should happily work for both the kernel and the userspace. Have
> you tried that? I might be wrong - it's quite some time I haven't used
> uapi headers.

Yep, that works for the kernel, thanks. However, bitops.h remains as we
don't have bitops.h for the userspace.

> Otherwise we might need to find a different solution - or live we what
> we have now :)

Yeah it works normally as the includes leak through other include files.
But I prefer explicit includes. I am thinking about some solution for
the bitops.h

Best Regards,

Markus
  

On Friday 26 December 2014 12:41:42 Markus Pargmann wrote:
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  packet.h | 5 +++++
>  1 file changed, 5 insertions(+)

Again, why is it a problem if these headers are missing ? Something not 
compiling ? Something else broken ? What are we fixing ?

Cheers,
Marek
  

On Sun, Dec 28, 2014 at 10:35:03AM +0800, Marek Lindner wrote:
> On Friday 26 December 2014 12:41:42 Markus Pargmann wrote:
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  packet.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> Again, why is it a problem if these headers are missing ? Something not 
> compiling ? Something else broken ? What are we fixing ?

This is fixing missing includes that I discovered when removing other
includes from c files. As this header uses defines from that include file
I think it should be explicitly listed. Will add more description to the
commit message.

Best regards,

Markus
  

Markus,

On 27/12/14 18:03, Markus Pargmann wrote:
> Hi Antonio,
> 
> On Sat, Dec 27, 2014 at 03:30:53PM +0100, Antonio Quartulli wrote:
>> Hi Markus,
>>
>> first of all you should know that I really appreciate your hard work.
>> Thank you very much for this!
>>
>> On 26/12/14 12:41, Markus Pargmann wrote:
>>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>>> ---
>>>  packet.h | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/packet.h b/packet.h
>>> index facd1feddd4e..5fd0d814b6de 100644
>>> --- a/packet.h
>>> +++ b/packet.h
>>> @@ -18,6 +18,11 @@
>>>  #ifndef _NET_BATMAN_ADV_PACKET_H_
>>>  #define _NET_BATMAN_ADV_PACKET_H_
>>>  
>>> +#ifdef __KERNEL__
>>> +#include <linux/bitops.h>
>>> +#include <uapi/linux/if_ether.h>
>>> +#endif /* __KERNEL__ */
>>
>> Unfortunately we can't do this :-(
>>
>> Patches sent to the kernel cannot contain conditional code on being in
>> the kernel or not (same is true for checks against a particular kernel
>> version - such code can't be in the kernel).
>>
>> Patches sent to the kernel must assume that they are only for the kernel
>> (and for that particular version we are sending the patches against).
> 
> Oh, I checked by grepping through the kernel before changing the patch
> like this.
> 	git grep "^#if.*def.*__KERNEL__" | grep -v include | wc -l
> 	145
> 
> So there are 145 uses of an ifdef __KERNEL__ outside of include
> directories (I excluded include directories as they may be exported to
> userspace). So I thought it would be ok.

these are probably pieces of code that have not yet been cleaned up.
Still, this does not allow us to introduce more code like this: David
(the networking tree maintainer) would refuse such additions.


Cheers,
  

On Wed, Dec 31, 2014 at 06:55:25PM +0100, Antonio Quartulli wrote:
> Markus,
> 
> On 27/12/14 18:03, Markus Pargmann wrote:
> > Hi Antonio,
> > 
> > On Sat, Dec 27, 2014 at 03:30:53PM +0100, Antonio Quartulli wrote:
> >> Hi Markus,
> >>
> >> first of all you should know that I really appreciate your hard work.
> >> Thank you very much for this!
> >>
> >> On 26/12/14 12:41, Markus Pargmann wrote:
> >>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> >>> ---
> >>>  packet.h | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/packet.h b/packet.h
> >>> index facd1feddd4e..5fd0d814b6de 100644
> >>> --- a/packet.h
> >>> +++ b/packet.h
> >>> @@ -18,6 +18,11 @@
> >>>  #ifndef _NET_BATMAN_ADV_PACKET_H_
> >>>  #define _NET_BATMAN_ADV_PACKET_H_
> >>>  
> >>> +#ifdef __KERNEL__
> >>> +#include <linux/bitops.h>
> >>> +#include <uapi/linux/if_ether.h>
> >>> +#endif /* __KERNEL__ */
> >>
> >> Unfortunately we can't do this :-(
> >>
> >> Patches sent to the kernel cannot contain conditional code on being in
> >> the kernel or not (same is true for checks against a particular kernel
> >> version - such code can't be in the kernel).
> >>
> >> Patches sent to the kernel must assume that they are only for the kernel
> >> (and for that particular version we are sending the patches against).
> > 
> > Oh, I checked by grepping through the kernel before changing the patch
> > like this.
> > 	git grep "^#if.*def.*__KERNEL__" | grep -v include | wc -l
> > 	145
> > 
> > So there are 145 uses of an ifdef __KERNEL__ outside of include
> > directories (I excluded include directories as they may be exported to
> > userspace). So I thought it would be ok.
> 
> these are probably pieces of code that have not yet been cleaned up.
> Still, this does not allow us to introduce more code like this: David
> (the networking tree maintainer) would refuse such additions.

Okay, thanks. I removed the bitops.h include for the moment as I don't
have an idea how to keep the header usable for the userspace
application.

Best regards,

Markus