Uploaded image for project: 'Lustre'
  1. Lustre
  2. LU-5011

lustre_idl.h again does not compile in user space

Details

    • Bug
    • Resolution: Fixed
    • Major
    • Lustre 2.10.0
    • 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

          Activity

            [LU-5011] lustre_idl.h again does not compile in user space

            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.

            morrone Christopher Morrone (Inactive) added a comment - 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.
            jhammond John Hammond added a comment -

            > 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?

            1. the __uXX from linux/types.h
            2. our __uXX from posix-types.h when linux/types.h is not available
            3. the uintXX_t from stdint.h?
            4. the lu_uintXX_t from libcfs/types.h?
            5. the obd_xxx from lustre_user.h
            6. none of the above
            7. some of the above
            8. all of the above
            jhammond John Hammond added a comment - > 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.

            adilger Andreas Dilger added a comment - 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.
            jhammond John Hammond added a comment - - edited

            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.

            jhammond John Hammond added a comment - - edited 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).

            morrone Christopher Morrone (Inactive) added a comment - 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
            
            jhammond John Hammond added a comment - 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.

            simmonsja James A Simmons added a comment - 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.

            morrone Christopher Morrone (Inactive) added a comment - 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
             */
            
            morrone Christopher Morrone (Inactive) added a comment - 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.

            jhammond John Hammond added a comment - 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.
            rread Robert Read added a comment - - edited

            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?

            rread Robert Read added a comment - - edited 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?

            People

              jhammond John Hammond
              morrone Christopher Morrone (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: