Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[James Simmons]

> On Tue, Dec 13, 2016 at 12:55:01AM +, Dilger, Andreas wrote:
> > On Dec 12, 2016, at 13:00, James Simmons  wrote:
> > > 
> > > 
> > >> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> > >>> In order for lustre_idl.h to be usable for both user
> > >>> land and kernel space it has to use the proper
> > >>> byteorder functions.
> > >> 
> > >> Why would userspace need/want all of these inline functions?  A uapi
> > >> header file should just have a the structures that are passed
> > >> user/kernel and any needed ioctls.  Why would they ever care about
> > >> strange byte flip functions and a ton of inline functions?
> > >> 
> > >> I don't think this is needed, of if it is, I really don't want to see
> > >> your crazy userspace code...
> > > 
> > > Sigh. More cleanups were done based on the idea this was okay. The
> > > reason this was does was when you look at the headers in
> > > include/uapi/linux you see a huge number of headers containing a bunch
> > > of inline function. To an outside project looking to merge their work
> > > into the kernel they would think this is okay. Hopefully all those
> > > broken headers will be cleaned up in the near future.
> > > Alright I will look to fixing up our tools to handle this requirement. 
> > 
> > These accessor functions are used by both the kernel and userspace
> > tools, and keeping them in the lustre_idl.h header avoids duplication
> > of code.  Similar usage exists in other filesystem related uapi headers
> > (e.g. auto_fs4.h, bcache.h, btrfs_tree.h, nilfs2_ondisk.h, swab.h, etc.).
> > 
> > That said, if there is an objection to keeping these macros/inline funcs
> > in the uapi headers, they still need to exist in the kernel and should
> > be kept in the lustre/include/lustre directory and we'll keep a separate
> > copy of the macros for userspace.
> 
> "simple" accessors/setters are fine, but these start to get complex, you
> are using unlikely, and debug macros and lots of other fun stuff.  Do
> all other filesystems also do complex stuff like ostid_to_fid()?

So the rejection of the byteorder patch was more due to the state of 
headers than the patch itself. I do have other patches with the cleanup
of debugging macros etc but I was submitting the change one change at a
time. I will post what cleanups I was looking to do for  lustre_ostid.h 
and lustre_fid.h UAPI headers. This way you can give feedback on what is 
okay and what has to change.

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[James Simmons]

> On Tue, Dec 13, 2016 at 12:55:01AM +, Dilger, Andreas wrote:
> > On Dec 12, 2016, at 13:00, James Simmons  wrote:
> > > 
> > > 
> > >> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> > >>> In order for lustre_idl.h to be usable for both user
> > >>> land and kernel space it has to use the proper
> > >>> byteorder functions.
> > >> 
> > >> Why would userspace need/want all of these inline functions?  A uapi
> > >> header file should just have a the structures that are passed
> > >> user/kernel and any needed ioctls.  Why would they ever care about
> > >> strange byte flip functions and a ton of inline functions?
> > >> 
> > >> I don't think this is needed, of if it is, I really don't want to see
> > >> your crazy userspace code...
> > > 
> > > Sigh. More cleanups were done based on the idea this was okay. The
> > > reason this was does was when you look at the headers in
> > > include/uapi/linux you see a huge number of headers containing a bunch
> > > of inline function. To an outside project looking to merge their work
> > > into the kernel they would think this is okay. Hopefully all those
> > > broken headers will be cleaned up in the near future.
> > > Alright I will look to fixing up our tools to handle this requirement. 
> > 
> > These accessor functions are used by both the kernel and userspace
> > tools, and keeping them in the lustre_idl.h header avoids duplication
> > of code.  Similar usage exists in other filesystem related uapi headers
> > (e.g. auto_fs4.h, bcache.h, btrfs_tree.h, nilfs2_ondisk.h, swab.h, etc.).
> > 
> > That said, if there is an objection to keeping these macros/inline funcs
> > in the uapi headers, they still need to exist in the kernel and should
> > be kept in the lustre/include/lustre directory and we'll keep a separate
> > copy of the macros for userspace.
> 
> "simple" accessors/setters are fine, but these start to get complex, you
> are using unlikely, and debug macros and lots of other fun stuff.  Do
> all other filesystems also do complex stuff like ostid_to_fid()?

So the rejection of the byteorder patch was more due to the state of 
headers than the patch itself. I do have other patches with the cleanup
of debugging macros etc but I was submitting the change one change at a
time. I will post what cleanups I was looking to do for  lustre_ostid.h 
and lustre_fid.h UAPI headers. This way you can give feedback on what is 
okay and what has to change.

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Dan Carpenter]

On Tue, Dec 13, 2016 at 11:14:26AM -0500, Oleg Drokin wrote:
> 
> On Dec 13, 2016, at 3:31 AM, Dan Carpenter wrote:
> 
> > It used to be that great swathes of Lustre were used in both user space
> > and kernel space.  We had huge unused modules in the kernel that were
> > only used for user space.
> 
> Huh?
> There was nothing of the sort.
> There were huge parts of code that were used by the server, but sue to no 
> server
> in staging client, ended up being unused, though.
> 

Oh.  Right,  that's it.

regards,
dan carpenter

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Dan Carpenter]

On Tue, Dec 13, 2016 at 11:14:26AM -0500, Oleg Drokin wrote:
> 
> On Dec 13, 2016, at 3:31 AM, Dan Carpenter wrote:
> 
> > It used to be that great swathes of Lustre were used in both user space
> > and kernel space.  We had huge unused modules in the kernel that were
> > only used for user space.
> 
> Huh?
> There was nothing of the sort.
> There were huge parts of code that were used by the server, but sue to no 
> server
> in staging client, ended up being unused, though.
> 

Oh.  Right,  that's it.

regards,
dan carpenter

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Oleg Drokin]

On Dec 13, 2016, at 3:31 AM, Dan Carpenter wrote:

> It used to be that great swathes of Lustre were used in both user space
> and kernel space.  We had huge unused modules in the kernel that were
> only used for user space.

Huh?
There was nothing of the sort.
There were huge parts of code that were used by the server, but sue to no server
in staging client, ended up being unused, though.

There were also (much smaller) bits that were supporting userspace client
(that is, a library that was able to mount lustre servers completely from
userspace by hijacking libc calls), but that was mostly gone by the time
we got into staging anyway.

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Oleg Drokin]

On Dec 13, 2016, at 3:31 AM, Dan Carpenter wrote:

> It used to be that great swathes of Lustre were used in both user space
> and kernel space.  We had huge unused modules in the kernel that were
> only used for user space.

Huh?
There was nothing of the sort.
There were huge parts of code that were used by the server, but sue to no server
in staging client, ended up being unused, though.

There were also (much smaller) bits that were supporting userspace client
(that is, a library that was able to mount lustre servers completely from
userspace by hijacking libc calls), but that was mostly gone by the time
we got into staging anyway.

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Dan Carpenter]

It used to be that great swathes of Lustre were used in both user space
and kernel space.  We had huge unused modules in the kernel that were
only used for user space.

regards,
dan carpenter

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Dan Carpenter]

It used to be that great swathes of Lustre were used in both user space
and kernel space.  We had huge unused modules in the kernel that were
only used for user space.

regards,
dan carpenter

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Greg Kroah-Hartman]

On Tue, Dec 13, 2016 at 12:55:01AM +, Dilger, Andreas wrote:
> On Dec 12, 2016, at 13:00, James Simmons  wrote:
> > 
> > 
> >> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> >>> In order for lustre_idl.h to be usable for both user
> >>> land and kernel space it has to use the proper
> >>> byteorder functions.
> >> 
> >> Why would userspace need/want all of these inline functions?  A uapi
> >> header file should just have a the structures that are passed
> >> user/kernel and any needed ioctls.  Why would they ever care about
> >> strange byte flip functions and a ton of inline functions?
> >> 
> >> I don't think this is needed, of if it is, I really don't want to see
> >> your crazy userspace code...
> > 
> > Sigh. More cleanups were done based on the idea this was okay. The
> > reason this was does was when you look at the headers in
> > include/uapi/linux you see a huge number of headers containing a bunch
> > of inline function. To an outside project looking to merge their work
> > into the kernel they would think this is okay. Hopefully all those
> > broken headers will be cleaned up in the near future.
> > Alright I will look to fixing up our tools to handle this requirement. 
> 
> These accessor functions are used by both the kernel and userspace
> tools, and keeping them in the lustre_idl.h header avoids duplication
> of code.  Similar usage exists in other filesystem related uapi headers
> (e.g. auto_fs4.h, bcache.h, btrfs_tree.h, nilfs2_ondisk.h, swab.h, etc.).
> 
> That said, if there is an objection to keeping these macros/inline funcs
> in the uapi headers, they still need to exist in the kernel and should
> be kept in the lustre/include/lustre directory and we'll keep a separate
> copy of the macros for userspace.

"simple" accessors/setters are fine, but these start to get complex, you
are using unlikely, and debug macros and lots of other fun stuff.  Do
all other filesystems also do complex stuff like ostid_to_fid()?

thanks,

greg k-h

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Greg Kroah-Hartman]

On Tue, Dec 13, 2016 at 12:55:01AM +, Dilger, Andreas wrote:
> On Dec 12, 2016, at 13:00, James Simmons  wrote:
> > 
> > 
> >> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> >>> In order for lustre_idl.h to be usable for both user
> >>> land and kernel space it has to use the proper
> >>> byteorder functions.
> >> 
> >> Why would userspace need/want all of these inline functions?  A uapi
> >> header file should just have a the structures that are passed
> >> user/kernel and any needed ioctls.  Why would they ever care about
> >> strange byte flip functions and a ton of inline functions?
> >> 
> >> I don't think this is needed, of if it is, I really don't want to see
> >> your crazy userspace code...
> > 
> > Sigh. More cleanups were done based on the idea this was okay. The
> > reason this was does was when you look at the headers in
> > include/uapi/linux you see a huge number of headers containing a bunch
> > of inline function. To an outside project looking to merge their work
> > into the kernel they would think this is okay. Hopefully all those
> > broken headers will be cleaned up in the near future.
> > Alright I will look to fixing up our tools to handle this requirement. 
> 
> These accessor functions are used by both the kernel and userspace
> tools, and keeping them in the lustre_idl.h header avoids duplication
> of code.  Similar usage exists in other filesystem related uapi headers
> (e.g. auto_fs4.h, bcache.h, btrfs_tree.h, nilfs2_ondisk.h, swab.h, etc.).
> 
> That said, if there is an objection to keeping these macros/inline funcs
> in the uapi headers, they still need to exist in the kernel and should
> be kept in the lustre/include/lustre directory and we'll keep a separate
> copy of the macros for userspace.

"simple" accessors/setters are fine, but these start to get complex, you
are using unlikely, and debug macros and lots of other fun stuff.  Do
all other filesystems also do complex stuff like ostid_to_fid()?

thanks,

greg k-h

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Dilger, Andreas]

On Dec 12, 2016, at 13:00, James Simmons  wrote:
> 
> 
>> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
>>> In order for lustre_idl.h to be usable for both user
>>> land and kernel space it has to use the proper
>>> byteorder functions.
>> 
>> Why would userspace need/want all of these inline functions?  A uapi
>> header file should just have a the structures that are passed
>> user/kernel and any needed ioctls.  Why would they ever care about
>> strange byte flip functions and a ton of inline functions?
>> 
>> I don't think this is needed, of if it is, I really don't want to see
>> your crazy userspace code...
> 
> Sigh. More cleanups were done based on the idea this was okay. The
> reason this was does was when you look at the headers in
> include/uapi/linux you see a huge number of headers containing a bunch
> of inline function. To an outside project looking to merge their work
> into the kernel they would think this is okay. Hopefully all those
> broken headers will be cleaned up in the near future.
> Alright I will look to fixing up our tools to handle this requirement. 

These accessor functions are used by both the kernel and userspace
tools, and keeping them in the lustre_idl.h header avoids duplication
of code.  Similar usage exists in other filesystem related uapi headers
(e.g. auto_fs4.h, bcache.h, btrfs_tree.h, nilfs2_ondisk.h, swab.h, etc.).

That said, if there is an objection to keeping these macros/inline funcs
in the uapi headers, they still need to exist in the kernel and should
be kept in the lustre/include/lustre directory and we'll keep a separate
copy of the macros for userspace.

Cheers, Andreas

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Dilger, Andreas]

On Dec 12, 2016, at 13:00, James Simmons  wrote:
> 
> 
>> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
>>> In order for lustre_idl.h to be usable for both user
>>> land and kernel space it has to use the proper
>>> byteorder functions.
>> 
>> Why would userspace need/want all of these inline functions?  A uapi
>> header file should just have a the structures that are passed
>> user/kernel and any needed ioctls.  Why would they ever care about
>> strange byte flip functions and a ton of inline functions?
>> 
>> I don't think this is needed, of if it is, I really don't want to see
>> your crazy userspace code...
> 
> Sigh. More cleanups were done based on the idea this was okay. The
> reason this was does was when you look at the headers in
> include/uapi/linux you see a huge number of headers containing a bunch
> of inline function. To an outside project looking to merge their work
> into the kernel they would think this is okay. Hopefully all those
> broken headers will be cleaned up in the near future.
> Alright I will look to fixing up our tools to handle this requirement. 

These accessor functions are used by both the kernel and userspace
tools, and keeping them in the lustre_idl.h header avoids duplication
of code.  Similar usage exists in other filesystem related uapi headers
(e.g. auto_fs4.h, bcache.h, btrfs_tree.h, nilfs2_ondisk.h, swab.h, etc.).

That said, if there is an objection to keeping these macros/inline funcs
in the uapi headers, they still need to exist in the kernel and should
be kept in the lustre/include/lustre directory and we'll keep a separate
copy of the macros for userspace.

Cheers, Andreas

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Greg Kroah-Hartman]

On Mon, Dec 12, 2016 at 08:00:02PM +, James Simmons wrote:
> 
> > On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> > > In order for lustre_idl.h to be usable for both user
> > > land and kernel space it has to use the proper
> > > byteorder functions.
> > 
> > Why would userspace need/want all of these inline functions?  A uapi
> > header file should just have a the structures that are passed
> > user/kernel and any needed ioctls.  Why would they ever care about
> > strange byte flip functions and a ton of inline functions?
> > 
> > I don't think this is needed, of if it is, I really don't want to see
> > your crazy userspace code...
> 
> Sigh. More cleanups were done based on the idea this was okay. The reason
> this was does was when you look at the headers in include/uapi/linux you
> see a huge number of headers containing a bunch of inline function. To
> an outside project looking to merge their work into the kernel they would
> think this is okay. Hopefully all those broken headers will be cleaned
> up in the near future. Alright I will look to fixing up our tools to 
> handle this requirement. 

But why do you need this type of stuff at all for your userspace code?
Why do they need these "complex" inline functions?  That implies that
there is duplicated logic on both sides of the user/kernel boundry,
shouldn't that be resolved somehow?

thanks,

greg k-h

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Greg Kroah-Hartman]

On Mon, Dec 12, 2016 at 08:00:02PM +, James Simmons wrote:
> 
> > On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> > > In order for lustre_idl.h to be usable for both user
> > > land and kernel space it has to use the proper
> > > byteorder functions.
> > 
> > Why would userspace need/want all of these inline functions?  A uapi
> > header file should just have a the structures that are passed
> > user/kernel and any needed ioctls.  Why would they ever care about
> > strange byte flip functions and a ton of inline functions?
> > 
> > I don't think this is needed, of if it is, I really don't want to see
> > your crazy userspace code...
> 
> Sigh. More cleanups were done based on the idea this was okay. The reason
> this was does was when you look at the headers in include/uapi/linux you
> see a huge number of headers containing a bunch of inline function. To
> an outside project looking to merge their work into the kernel they would
> think this is okay. Hopefully all those broken headers will be cleaned
> up in the near future. Alright I will look to fixing up our tools to 
> handle this requirement. 

But why do you need this type of stuff at all for your userspace code?
Why do they need these "complex" inline functions?  That implies that
there is duplicated logic on both sides of the user/kernel boundry,
shouldn't that be resolved somehow?

thanks,

greg k-h

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[James Simmons]

> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> > In order for lustre_idl.h to be usable for both user
> > land and kernel space it has to use the proper
> > byteorder functions.
> 
> Why would userspace need/want all of these inline functions?  A uapi
> header file should just have a the structures that are passed
> user/kernel and any needed ioctls.  Why would they ever care about
> strange byte flip functions and a ton of inline functions?
> 
> I don't think this is needed, of if it is, I really don't want to see
> your crazy userspace code...

Sigh. More cleanups were done based on the idea this was okay. The reason
this was does was when you look at the headers in include/uapi/linux you
see a huge number of headers containing a bunch of inline function. To
an outside project looking to merge their work into the kernel they would
think this is okay. Hopefully all those broken headers will be cleaned
up in the near future. Alright I will look to fixing up our tools to 
handle this requirement. 

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[James Simmons]

> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> > In order for lustre_idl.h to be usable for both user
> > land and kernel space it has to use the proper
> > byteorder functions.
> 
> Why would userspace need/want all of these inline functions?  A uapi
> header file should just have a the structures that are passed
> user/kernel and any needed ioctls.  Why would they ever care about
> strange byte flip functions and a ton of inline functions?
> 
> I don't think this is needed, of if it is, I really don't want to see
> your crazy userspace code...

Sigh. More cleanups were done based on the idea this was okay. The reason
this was does was when you look at the headers in include/uapi/linux you
see a huge number of headers containing a bunch of inline function. To
an outside project looking to merge their work into the kernel they would
think this is okay. Hopefully all those broken headers will be cleaned
up in the near future. Alright I will look to fixing up our tools to 
handle this requirement. 

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Greg Kroah-Hartman]

On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> In order for lustre_idl.h to be usable for both user
> land and kernel space it has to use the proper
> byteorder functions.

Why would userspace need/want all of these inline functions?  A uapi
header file should just have a the structures that are passed
user/kernel and any needed ioctls.  Why would they ever care about
strange byte flip functions and a ton of inline functions?

I don't think this is needed, of if it is, I really don't want to see
your crazy userspace code...

thanks,

greg k-h

Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

[Greg Kroah-Hartman]

On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> In order for lustre_idl.h to be usable for both user
> land and kernel space it has to use the proper
> byteorder functions.

Why would userspace need/want all of these inline functions?  A uapi
header file should just have a the structures that are passed
user/kernel and any needed ioctls.  Why would they ever care about
strange byte flip functions and a ton of inline functions?

I don't think this is needed, of if it is, I really don't want to see
your crazy userspace code...

thanks,

greg k-h