Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
> 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
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
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
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
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
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
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
> 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
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