Details
-
Bug
-
Resolution: Fixed
-
Major
-
Lustre 2.4.0, Lustre 2.4.1, Lustre 2.5.0, Lustre 2.4.2, Lustre 2.5.1, Lustre 2.4.3
-
3
-
13871
Description
New ticket for the the recurring problem of lustre_idl.h not compiling in user-space. Conversation began in LU-1606, but we want to have a separate ticket for Lustre 2.6.0.
Attachments
Issue Links
- is blocked by
-
-
- Resolved
-
-
-
- Resolved
-
-
-
- Closed
-
-
-
- Closed
-
- is related to
-
-
- Resolved
-
-
-
- Closed
-
-
LU-6245 Untangle userland and kernel space support for libcfs
-
- Resolved
-
- is related to
-
-
- Resolved
-
-
-
- Closed
-
-
-
- Resolved
-
Activity
Seems like an odd choice. But Linux is just odd that way sometimes, I guess.
I guess I'm fine with use of the __u* as long as it is for things that interface directly with the kernel.
Actually, the reverse is true - the __u* versions are for interface usage, and u* is for kernel internal use. Please don't go the opposite way from the kernel.
I was primary referring to 4, lu_uintXX_t. I think that takes us in the wrong direction.
I find __uXX types a little less bothersome. But if anything, it sees like we should use the uXX and sXX forms rather than the forms that are preceded with underscores when we wish to share with userspace.
Overall I am still thinking uintXX_t is probably the best way to go.
> I do understand your point about PRIx64 type macros, and perhaps glossed over it a bit too easily in my previously reply. But I believe we should deal with the issue of displaying the value separately from this decision. After all, we already have to do a whole bunch of work to create macros for printk in the kernel; what I am proposing would be no worse. On the other hand your lu_* suggestion then exposes all of that extra work to user space.
It's already done and it's less work than before. See http://review.whamcloud.com/#/c/10273.
By introducing "new types to userspace" which types do you mean?
- the __uXX from linux/types.h
- our __uXX from posix-types.h when linux/types.h is not available
- the uintXX_t from stdint.h?
- the lu_uintXX_t from libcfs/types.h?
- the obd_xxx from lustre_user.h
- none of the above
- some of the above
- all of the above
I'm not necessarily keen on introducing new types to userspace instead of __u32 and friends. Other packages that interface with the kernel (e.g. e2fsprogs) also need to use __u* types for ioctls. See http://review.whamcloud.com/10258 for a relatively simple start to this. It doesn't do as much cleanup as John's patch.
Please see http://review.whamcloud.com/#/c/10280/ for what happens when we use uin32_t in the headers. The issue now is that if __KERNEL__ is defined then __u64 and uint64_t are the same type (unsigned long long). If __KERNEL__ is not defined then one is unsigned long and the other is unsigned long long. Thus we have a huge flag day where all of user space needs to be rewritten to not use __[us]XX. For sanity sake this also means not using LPX64. Whatever changes we make we still need to stay friends with GCC.
Regarding the fuse headers: yes they look very nice. The difference I see is that they probably share very little code between user space and kernel space. We share a lot. It's not great that we do, but we do. Please remember that there are limits to how much change I can convince the other developers to accept in a short time.
But then uint64_t is defined using an inequivalent primitive type.
Yes, that is true. They are supposed to be different. The same primitive type is not necessarily the same size on the same platform in both kernel and user space. You cannot assume that they are, unfortunately. Which is why you want to let the kernel header define the value in the kernel, and let the user space glibc header define the value in user space. Then you wind up with the correctly sized field wherever you are.
I do understand your point about PRIx64 type macros, and perhaps glossed over it a bit too easily in my previously reply. But I believe we should deal with the issue of displaying the value separately from this decision. After all, we already have to do a whole bunch of work to create macros for printk in the kernel; what I am proposing would be no worse. On the other hand your lu_* suggestion then exposes all of that extra work to user space.
With my suggestion applications that are fully in user space get to use the easy PRI* macros, or any other method of their choosing. The in-kernel code that wants to display the values is no worse off than it is today (a whole bunch of painful to maintain, but necessary, macros).
I tried to use uint64_t et al in the packaged headers. It's just too subtle and too broken. When userspace includes linux/types.h it doesn't get uint64_t. So userspace would need to include stdint.h. But then uint64_t is defined using an inequivalent primitive type.
t:~# uname -r 2.6.32-431.5.1.el6.lustre.x86_64 t:~# cat /etc/redhat-release CentOS release 6.4 (Final) t:~# cat test-linux.c #include <linux/types.h> uint64_t u; t:~# gcc -Wall -c test-linux.c test-linux.c:3: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘u’ t:~# cat test-stdint.c #include <stdint.h> uint64_t u; t:~# gcc -Wall -c test-stdint.c -g t:~# gdb test-stdint.o GNU gdb (GDB) Red Hat Enterprise Linux (7.2-60.el6) ... Reading symbols from /root/test-stdint.o...done. (gdb) ptype u type = long unsigned int t:~# gdb lustre-release/lustre/llite/lustre.ko GNU gdb (GDB) Red Hat Enterprise Linux (7.2-60.el6) ... Reading symbols from /root/lustre-release/lustre/llite/lustre.ko...done. (gdb) ptype uint64_t type = long long unsigned int
No please don't make my eyes bleed with lu_* cfs_* types. Chris is right in that the kernel types were developed before the C99 spec. It not true that C99 types are discouraged. In fact I quote from Documentation/CodingStyle
----------------------------------------------------------------------------------------------------------------------------
(d) New types which are identical to standard C99 types, in certain
exceptional circumstances.
Although it would only take a short amount of time for the eyes and
brain to become accustomed to the standard types like 'uint32_t',
some people object to their use anyway.
Therefore, the Linux-specific 'u8/u16/u32/u64' types and their
signed equivalents which are identical to standard types are
permitted – although they are not mandatory in new code of your
own.
When editing existing code which already uses one or the other set
of types, you should conform to the existing choices in that code.
------------------------------------------------------------------------------------------------------------------
Any non Lustre code I submit to the kernel always uses the C99 types. As for C99 types being broken that I really doubt. Their are millions of applications that depend on glibc getting it right. The last point to make is
we don't need to use linux types for kernel to userland interfaces as well. It it perfectly sane to use C99 types.
In fact to see a beautiful example of this take a look at
linux-source-tree/include/uapi/linux/fuse.h
Not a single __uXX crap thing there. Then take a look fuse_do_ioctl in linux-source-tree/fs/fuse/file.c. Again no __uXX crap. Lastly any code pushed upstream with special types will be immediately rejected.
Usually I'm pretty non bias on most things but this is not one of them.
without depending on config.h
By the way, I entirely agree with that point. User space should not depend on Lustre's config.h. I squashed some attempts to add further dependencies on that in the past year or two.
Looking at a FC13 test system I have, <linux/types.h> is part of kernel-headers*.rpm, which is required by glibc-headers, which is required by glibc-headers, which is required by glibc-devel, which is required by gcc, so it does indeed seem that <linux/types.h> is going to be installed on any system that is compiling something on Linux.
I think it makes sense to go back to http://review.whamcloud.com/#/c/10258/1/libcfs/include/libcfs/posix/posix-types.h which includes <linux/types.h> on any Linux system.