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
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.
We don't control them (whether they come from linux/types.h or from stdint.h).
I think that is a good thing. This is a really basic, well-known data type of explicitly fixed size. We'll probably get them wrong somewhere on some architecture if we do them ourselves. We have gotten very similar things wrong in the past.
We should try to avoid including them through our headers if at all possible.
I am not sure that I understand the rationale for this assertion. Maybe a longer explanation is needed.
In the kernel uint64_t is defined to be __u64 which is usually unsigned long long.
I am not sure that I understand your point. That is the type that winds up being unsigned and 64 bits in the kernel for that particular architecture. Someone else figured that out for us so we don't have to. I see that as a good thing.
In stdint.h on x86_64 uint64_t is defined to be unsigned long.
I would use the same argument as above. That is good thing. They figured out which type was needed to deliver an unsigned 64 bit storage space. We will be forced to do exactly the same thing with our definitions. If we don't we will wind up with the wrong storage size in some places on some architectures.
If we tried to use unsigned long long in user space when unsigned long was the correct size, we could wind up using a 128 bit field where we wanted a 64 bit field. We are using fixed size data types for a reason when we define those structures. They need to be a particular fixed size. It is far better to use the work that the Linux and C language communities have done to define those values. They are much less likely to make a mistake then we are since their developer and user bases are so much larger than ours.
The kernel does not define the PRIx64 type macros.
That is certainly true. We can't expect everything from stdint.h to be available in the kernel. I didn't mean to imply that they would be. Your point is a good one, and I think we can address that by use of a good code comment. Something like this perhaps:
/* Note that many of definitions in stdint.h definitions are _not_ available in the kernel. * It is safe to use these eight types: * int8_t, int16_t, int32_t, int64_t, uint8_t, uint16_t, uint32_t, uint64_t */
Actually in the patch we /do/ use those type to go between user and kernel space. We just don't want to package header that depend on or export those types (again LU-5018). Moreover you'll recall that all "identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use" by the C standard library.
It would definitely be appropriate for libraries that sit on top of the kernel interface to use <stdint.h> types in their higher level API. I don't think it's currently the right thing to do in the the interface between kernel and userspace.
I believe the normal thing to do (again, see fiemap) is to use linux/types.h (the __* types) for data being exchanged between user and kernel space. Why not just use that instead of posix-types.h or inventing new ones?
Yes, I've seen the typedefs for the stdint.h types in types.h. However their use is generally discouraged in kernel code. There are also a few issues I have found with them:
- We don't control them (whether they come from linux/types.h or from stdint.h).
- We should try to avoid including them through our headers if at all possible.
- In the kernel uint64_t is defined to be __u64 which is usually unsigned long long.
- In stdint.h on x86_64 uint64_t is defined to be unsigned long.
- The kernel does not define the PRIx64 type macros.
I would like it if our library header could be written using C primitive types, stddef.h, and sys/types.h. But this will take some time. In the mean time, using names that we control, without depending on config.h, and without bringing in the __uXX names (see LU-5018) seems like a good first step.
John, I have a suggestion for patch 10273.
The fixed-size data types:
- int[8,16,32,64]_t
- uint[8,16,32,64]_t
were standardized in user space in C99 and defined in the header stdint.h. Granted, the kernel is still largly C89, and doesn't implement the full standard.
The kernel probably didn't have those same defines back when we started using the double-underscore names, but it looks to me like the kernel made a nod towards compatibility a while back by defining those same eight fixed-sized types in include/linux/types.h.
So perhaps instead of creating the new lu_* types, we could just use the more well known C99 names in both user- and kernel-space. For a header that needs to be included both places, the include snippet would be:
#ifdef __KERNEL__
#include <linux/types.h>
#else
#include <stdint.h>
#endif
So now we have:
- http://review.whamcloud.com/#/c/10215 to unpackage several headers
- http://review.whamcloud.com/#/c/10273 to make packaged headers not depend on config.h (rough draft)
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.