Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Mikko Rapeli]

On Wed, Aug 24, 2016 at 08:57:21AM +0200, Frans Klaver wrote:
> On Tue, Aug 23, 2016 at 10:03 AM, Frans Klaver  wrote:
> > On Tue, Aug 23, 2016 at 9:05 AM, David Miller  wrote:
> >> From: Frans Klaver 
> >> Date: Tue, 23 Aug 2016 09:03:20 +0200
> >>
> >>> On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
>  From: Mikko Rapeli 
>  Date: Mon, 22 Aug 2016 20:32:44 +0200
> 
> > Fixes userspace compiler error:
> >
> > error: ‘IFNAMSIZ’ undeclared here (not in a function)
> >
> > Suggested by Frans Klaver  on lkml message
> > <20150530195223.ga15...@bugger.home>.
> >
> > Signed-off-by: Mikko Rapeli 
> 
>  IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
>  to move it to the hdlc header instead of having the hdlc header
>  include linux/if.h
> >>>
> >>> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
> >>> define IFNAMSIZ before doing so.
> >>
> >> That's not acceptable.  Use forward declarations or similar to avoid
> >> the circular dependency.
> >>
> >> IFNAMSIZ belongs in linux/if.h, please keep it there.
> >
> > I went back to one of the previous patch sets, but couldn't find why
> > the circular dependency had to be broken. So if this can be fixed by
> > including linux/if.h instead, I'm all for it.
> 
> Alright, so the core of the 'problem' is that the structs in
> hdlc/ioctl.h are typedefs of anonymous structs, and linux/if.h points
> to those types. We can't really forward declare these structs unless
> we name them, so the proper approach would be to name them and use
> forward declarations in linux/if.h. hdlc/ioctl.h can then include
> linux/if.h. linux/if.h should probably keep including hdlc/ioctl.h to
> keep depending application builds from breaking.

Thanks! I will do this and send a new patch.

-Mikko

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Mikko Rapeli]

On Wed, Aug 24, 2016 at 08:57:21AM +0200, Frans Klaver wrote:
> On Tue, Aug 23, 2016 at 10:03 AM, Frans Klaver  wrote:
> > On Tue, Aug 23, 2016 at 9:05 AM, David Miller  wrote:
> >> From: Frans Klaver 
> >> Date: Tue, 23 Aug 2016 09:03:20 +0200
> >>
> >>> On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
>  From: Mikko Rapeli 
>  Date: Mon, 22 Aug 2016 20:32:44 +0200
> 
> > Fixes userspace compiler error:
> >
> > error: ‘IFNAMSIZ’ undeclared here (not in a function)
> >
> > Suggested by Frans Klaver  on lkml message
> > <20150530195223.ga15...@bugger.home>.
> >
> > Signed-off-by: Mikko Rapeli 
> 
>  IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
>  to move it to the hdlc header instead of having the hdlc header
>  include linux/if.h
> >>>
> >>> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
> >>> define IFNAMSIZ before doing so.
> >>
> >> That's not acceptable.  Use forward declarations or similar to avoid
> >> the circular dependency.
> >>
> >> IFNAMSIZ belongs in linux/if.h, please keep it there.
> >
> > I went back to one of the previous patch sets, but couldn't find why
> > the circular dependency had to be broken. So if this can be fixed by
> > including linux/if.h instead, I'm all for it.
> 
> Alright, so the core of the 'problem' is that the structs in
> hdlc/ioctl.h are typedefs of anonymous structs, and linux/if.h points
> to those types. We can't really forward declare these structs unless
> we name them, so the proper approach would be to name them and use
> forward declarations in linux/if.h. hdlc/ioctl.h can then include
> linux/if.h. linux/if.h should probably keep including hdlc/ioctl.h to
> keep depending application builds from breaking.

Thanks! I will do this and send a new patch.

-Mikko

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 10:03 AM, Frans Klaver  wrote:
> On Tue, Aug 23, 2016 at 9:05 AM, David Miller  wrote:
>> From: Frans Klaver 
>> Date: Tue, 23 Aug 2016 09:03:20 +0200
>>
>>> On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
 From: Mikko Rapeli 
 Date: Mon, 22 Aug 2016 20:32:44 +0200

> Fixes userspace compiler error:
>
> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>
> Suggested by Frans Klaver  on lkml message
> <20150530195223.ga15...@bugger.home>.
>
> Signed-off-by: Mikko Rapeli 

 IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
 to move it to the hdlc header instead of having the hdlc header
 include linux/if.h
>>>
>>> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
>>> define IFNAMSIZ before doing so.
>>
>> That's not acceptable.  Use forward declarations or similar to avoid
>> the circular dependency.
>>
>> IFNAMSIZ belongs in linux/if.h, please keep it there.
>
> I went back to one of the previous patch sets, but couldn't find why
> the circular dependency had to be broken. So if this can be fixed by
> including linux/if.h instead, I'm all for it.

Alright, so the core of the 'problem' is that the structs in
hdlc/ioctl.h are typedefs of anonymous structs, and linux/if.h points
to those types. We can't really forward declare these structs unless
we name them, so the proper approach would be to name them and use
forward declarations in linux/if.h. hdlc/ioctl.h can then include
linux/if.h. linux/if.h should probably keep including hdlc/ioctl.h to
keep depending application builds from breaking.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 10:03 AM, Frans Klaver  wrote:
> On Tue, Aug 23, 2016 at 9:05 AM, David Miller  wrote:
>> From: Frans Klaver 
>> Date: Tue, 23 Aug 2016 09:03:20 +0200
>>
>>> On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
 From: Mikko Rapeli 
 Date: Mon, 22 Aug 2016 20:32:44 +0200

> Fixes userspace compiler error:
>
> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>
> Suggested by Frans Klaver  on lkml message
> <20150530195223.ga15...@bugger.home>.
>
> Signed-off-by: Mikko Rapeli 

 IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
 to move it to the hdlc header instead of having the hdlc header
 include linux/if.h
>>>
>>> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
>>> define IFNAMSIZ before doing so.
>>
>> That's not acceptable.  Use forward declarations or similar to avoid
>> the circular dependency.
>>
>> IFNAMSIZ belongs in linux/if.h, please keep it there.
>
> I went back to one of the previous patch sets, but couldn't find why
> the circular dependency had to be broken. So if this can be fixed by
> including linux/if.h instead, I'm all for it.

Alright, so the core of the 'problem' is that the structs in
hdlc/ioctl.h are typedefs of anonymous structs, and linux/if.h points
to those types. We can't really forward declare these structs unless
we name them, so the proper approach would be to name them and use
forward declarations in linux/if.h. hdlc/ioctl.h can then include
linux/if.h. linux/if.h should probably keep including hdlc/ioctl.h to
keep depending application builds from breaking.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 9:05 AM, David Miller  wrote:
> From: Frans Klaver 
> Date: Tue, 23 Aug 2016 09:03:20 +0200
>
>> On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
>>> From: Mikko Rapeli 
>>> Date: Mon, 22 Aug 2016 20:32:44 +0200
>>>
 Fixes userspace compiler error:

 error: ‘IFNAMSIZ’ undeclared here (not in a function)

 Suggested by Frans Klaver  on lkml message
 <20150530195223.ga15...@bugger.home>.

 Signed-off-by: Mikko Rapeli 
>>>
>>> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
>>> to move it to the hdlc header instead of having the hdlc header
>>> include linux/if.h
>>
>> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
>> define IFNAMSIZ before doing so.
>
> That's not acceptable.  Use forward declarations or similar to avoid
> the circular dependency.
>
> IFNAMSIZ belongs in linux/if.h, please keep it there.

I went back to one of the previous patch sets, but couldn't find why
the circular dependency had to be broken. So if this can be fixed by
including linux/if.h instead, I'm all for it.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 9:05 AM, David Miller  wrote:
> From: Frans Klaver 
> Date: Tue, 23 Aug 2016 09:03:20 +0200
>
>> On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
>>> From: Mikko Rapeli 
>>> Date: Mon, 22 Aug 2016 20:32:44 +0200
>>>
 Fixes userspace compiler error:

 error: ‘IFNAMSIZ’ undeclared here (not in a function)

 Suggested by Frans Klaver  on lkml message
 <20150530195223.ga15...@bugger.home>.

 Signed-off-by: Mikko Rapeli 
>>>
>>> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
>>> to move it to the hdlc header instead of having the hdlc header
>>> include linux/if.h
>>
>> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
>> define IFNAMSIZ before doing so.
>
> That's not acceptable.  Use forward declarations or similar to avoid
> the circular dependency.
>
> IFNAMSIZ belongs in linux/if.h, please keep it there.

I went back to one of the previous patch sets, but couldn't find why
the circular dependency had to be broken. So if this can be fixed by
including linux/if.h instead, I'm all for it.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
> From: Mikko Rapeli 
> Date: Mon, 22 Aug 2016 20:32:44 +0200
>
>> Fixes userspace compiler error:
>>
>> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>>
>> Suggested by Frans Klaver  on lkml message
>> <20150530195223.ga15...@bugger.home>.
>>
>> Signed-off-by: Mikko Rapeli 
>
> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
> to move it to the hdlc header instead of having the hdlc header
> include linux/if.h

Circular references. linux/if.h includes hdlc/ioctl.h, and has to
define IFNAMSIZ before doing so. If IFNAMSIZ is moved to hdlc/ioctl.h,
it is still pulled in if one includes linux/if.h. What we gain is that
you can include hdlc/ioctl.h directly (which is what one of the tests
is already doing, iirc).

But yes, it should be explained in the commit message.

Thanks,
Frans

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Frans Klaver]

On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
> From: Mikko Rapeli 
> Date: Mon, 22 Aug 2016 20:32:44 +0200
>
>> Fixes userspace compiler error:
>>
>> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>>
>> Suggested by Frans Klaver  on lkml message
>> <20150530195223.ga15...@bugger.home>.
>>
>> Signed-off-by: Mikko Rapeli 
>
> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
> to move it to the hdlc header instead of having the hdlc header
> include linux/if.h

Circular references. linux/if.h includes hdlc/ioctl.h, and has to
define IFNAMSIZ before doing so. If IFNAMSIZ is moved to hdlc/ioctl.h,
it is still pulled in if one includes linux/if.h. What we gain is that
you can include hdlc/ioctl.h directly (which is what one of the tests
is already doing, iirc).

But yes, it should be explained in the commit message.

Thanks,
Frans

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[David Miller]

From: Frans Klaver 
Date: Tue, 23 Aug 2016 09:03:20 +0200

> On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
>> From: Mikko Rapeli 
>> Date: Mon, 22 Aug 2016 20:32:44 +0200
>>
>>> Fixes userspace compiler error:
>>>
>>> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>>>
>>> Suggested by Frans Klaver  on lkml message
>>> <20150530195223.ga15...@bugger.home>.
>>>
>>> Signed-off-by: Mikko Rapeli 
>>
>> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
>> to move it to the hdlc header instead of having the hdlc header
>> include linux/if.h
> 
> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
> define IFNAMSIZ before doing so.

That's not acceptable.  Use forward declarations or similar to avoid
the circular dependency.

IFNAMSIZ belongs in linux/if.h, please keep it there.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[David Miller]

From: Frans Klaver 
Date: Tue, 23 Aug 2016 09:03:20 +0200

> On Tue, Aug 23, 2016 at 1:30 AM, David Miller  wrote:
>> From: Mikko Rapeli 
>> Date: Mon, 22 Aug 2016 20:32:44 +0200
>>
>>> Fixes userspace compiler error:
>>>
>>> error: ‘IFNAMSIZ’ undeclared here (not in a function)
>>>
>>> Suggested by Frans Klaver  on lkml message
>>> <20150530195223.ga15...@bugger.home>.
>>>
>>> Signed-off-by: Mikko Rapeli 
>>
>> IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
>> to move it to the hdlc header instead of having the hdlc header
>> include linux/if.h
> 
> Circular references. linux/if.h includes hdlc/ioctl.h, and has to
> define IFNAMSIZ before doing so.

That's not acceptable.  Use forward declarations or similar to avoid
the circular dependency.

IFNAMSIZ belongs in linux/if.h, please keep it there.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[David Miller]

From: Mikko Rapeli 
Date: Mon, 22 Aug 2016 20:32:44 +0200

> Fixes userspace compiler error:
> 
> error: ‘IFNAMSIZ’ undeclared here (not in a function)
> 
> Suggested by Frans Klaver  on lkml message
> <20150530195223.ga15...@bugger.home>.
> 
> Signed-off-by: Mikko Rapeli 

IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
to move it to the hdlc header instead of having the hdlc header
include linux/if.h

And if your reason is legitimate, you have to add that explanation to
the patch commit log message.

Re: [PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[David Miller]

From: Mikko Rapeli 
Date: Mon, 22 Aug 2016 20:32:44 +0200

> Fixes userspace compiler error:
> 
> error: ‘IFNAMSIZ’ undeclared here (not in a function)
> 
> Suggested by Frans Klaver  on lkml message
> <20150530195223.ga15...@bugger.home>.
> 
> Signed-off-by: Mikko Rapeli 

IFNAMSIZ has to be in linux/if.h, you aren't explaining why you have
to move it to the hdlc header instead of having the hdlc header
include linux/if.h

And if your reason is legitimate, you have to add that explanation to
the patch commit log message.

[PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Mikko Rapeli]

Fixes userspace compiler error:

error: ‘IFNAMSIZ’ undeclared here (not in a function)

Suggested by Frans Klaver  on lkml message
<20150530195223.ga15...@bugger.home>.

Signed-off-by: Mikko Rapeli 
---
 include/uapi/linux/hdlc/ioctl.h | 5 +
 include/uapi/linux/if.h | 5 +
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/hdlc/ioctl.h b/include/uapi/linux/hdlc/ioctl.h
index 04bc027..4b7a7ed 100644
--- a/include/uapi/linux/hdlc/ioctl.h
+++ b/include/uapi/linux/hdlc/ioctl.h
@@ -1,6 +1,11 @@
 #ifndef __HDLC_IOCTL_H__
 #define __HDLC_IOCTL_H__
 
+#include  /* for compatibility with glibc */
+
+#if __UAPI_DEF_IF_IFNAMSIZ
+#defineIFNAMSIZ16
+#endif /* __UAPI_DEF_IF_IFNAMSIZ */
 
 #define GENERIC_HDLC_VERSION 4 /* For synchronization with sethdlc utility */
 
diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
index e601c8c..68bd205 100644
--- a/include/uapi/linux/if.h
+++ b/include/uapi/linux/if.h
@@ -23,12 +23,9 @@
 #include/* for "__kernel_caddr_t" et al */
 #include   /* for "struct sockaddr" et al  */
 #include /* for "__user" et al   */
+#include   /* for IFNAMSIZ */
 
-#if __UAPI_DEF_IF_IFNAMSIZ
-#defineIFNAMSIZ16
-#endif /* __UAPI_DEF_IF_IFNAMSIZ */
 #defineIFALIASZ256
-#include 
 
 /* For glibc compatibility. An empty enum does not compile. */
 #if __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO != 0 && \
-- 
2.8.1

[PATCH v05 27/72] linux/if.h linux/hdlc/ioctl.h: move IFNAMSIZ definition to hdlc/ioctl.h

[Mikko Rapeli]

Fixes userspace compiler error:

error: ‘IFNAMSIZ’ undeclared here (not in a function)

Suggested by Frans Klaver  on lkml message
<20150530195223.ga15...@bugger.home>.

Signed-off-by: Mikko Rapeli 
---
 include/uapi/linux/hdlc/ioctl.h | 5 +
 include/uapi/linux/if.h | 5 +
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/hdlc/ioctl.h b/include/uapi/linux/hdlc/ioctl.h
index 04bc027..4b7a7ed 100644
--- a/include/uapi/linux/hdlc/ioctl.h
+++ b/include/uapi/linux/hdlc/ioctl.h
@@ -1,6 +1,11 @@
 #ifndef __HDLC_IOCTL_H__
 #define __HDLC_IOCTL_H__
 
+#include  /* for compatibility with glibc */
+
+#if __UAPI_DEF_IF_IFNAMSIZ
+#defineIFNAMSIZ16
+#endif /* __UAPI_DEF_IF_IFNAMSIZ */
 
 #define GENERIC_HDLC_VERSION 4 /* For synchronization with sethdlc utility */
 
diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
index e601c8c..68bd205 100644
--- a/include/uapi/linux/if.h
+++ b/include/uapi/linux/if.h
@@ -23,12 +23,9 @@
 #include/* for "__kernel_caddr_t" et al */
 #include   /* for "struct sockaddr" et al  */
 #include /* for "__user" et al   */
+#include   /* for IFNAMSIZ */
 
-#if __UAPI_DEF_IF_IFNAMSIZ
-#defineIFNAMSIZ16
-#endif /* __UAPI_DEF_IF_IFNAMSIZ */
 #defineIFALIASZ256
-#include 
 
 /* For glibc compatibility. An empty enum does not compile. */
 #if __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO != 0 && \
-- 
2.8.1