On Saturday 06 March 2010 10:31:30 Denys Vlasenko wrote:
> On Saturday 06 March 2010 01:48, Rob Landley wrote:
> > > I received a pre-compiled busybox version 1.6.1 where I am able to make
> > > any configuration changes and compile and run the busybox on my
> > > hardware.
> > > However, I needed SMTP & POP support in my application, so I choose to
> > > upgrade to busybox 1.15.3.
> > >
> > > When I try to compile 1.15.3, I am getting the compile error as
> > > attached.
> >
> > Sigh. Years ago, I spent several months fighting with loop.c trying to
> > beat some _sanity_ out of it, and finally got it working reliably by
> > moving us to the 64-bit api in 2.6, which eliminated the horrible legacy
> > cruft.
> >
> > Then commit 9b1b62ad inexplicably decided to add #ifdefs and #defines for
> > BSD to an #include <linux/blah.h> file.
> >
> > Hands up everybdy else who spots a fundamental problem with the idea of
> > BSD #including a linux kernel header file ever under any circumstances,
> > and more so with the idea of them having some strange compatability
> > support for Linux- specific APIs but getting them WRONG and it somehow
> > being our problem?
> >
> > Try reverting that commit (or at least the bits that touch loop.c) and
> > see if that fixes it for you?
> >
> > Denys? Why did you do that?
>
> I assume you ask why this commit touched loop.c:
>
> http://git.busybox.net/busybox/commit/?id=9b1b62adc4e4c1e80d9f72180c6b7b1ea
>ef9f95a
>
> Here is the relevant part of the same diff with indentation changes
> omitted:
>
> --- busybox-b22bbfffec182997827b0a71eeb93ddafbde602c/libbb/loop.c
> +++ busybox-9b1b62adc4e4c1e80d9f72180c6b7b1eaef9f95a/libbb/loop.c
> @@ -7,19 +7,27 @@
> *
> * Licensed under the GPL v2 or later, see the file LICENSE in this
> tarball. */
> -
> #include "libbb.h"
> -
> -/* For 2.6, use the cleaned up header to get the 64 bit API. */
> #include <linux/version.h>
> +
> #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> +
> +/* For 2.6, use the cleaned up header to get the 64 bit API. */
> +/* linux/loop.h relies on __u64. Make sure we have that as a proper type
> + * until userspace is widely fixed. */
> +# if (defined __INTEL_COMPILER && !defined __GNUC__) \
> + || (defined __GNUC__ && defined __STRICT_ANSI__)
> +__extension__ typedef long long __s64;
> +__extension__ typedef unsigned long long __u64;
> +# endif
> #include <linux/loop.h>
> typedef struct loop_info64 bb_loop_info;
> #define BB_LOOP_SET_STATUS LOOP_SET_STATUS64
> #define BB_LOOP_GET_STATUS LOOP_GET_STATUS64
>
> -/* For 2.4 and earlier, use the 32 bit API (and don't trust the headers)
> */ #else
> +
> +/* For 2.4 and earlier, use the 32 bit API (and don't trust the headers)
> */ /* Stuff stolen from linux/loop.h for 2.4 and earlier kernels*/
> #include <linux/posix_types.h>
> #define LO_NAME_SIZE 64
>
>
> The only change here is that __s64 and __u64 are typedef'ed in some cases.
> I did it because a user reporter it did not work for him until he added it.
>
> Do you think it's wrong?
Yes, I think it's wrong. The toolchain that had a problem with that is
clearly broken, and cluttering up busybox's code for a brittle workaround for
a specific obviously broken toolchain isn't an improvement.
The __s64 and __u64 types are kernel internal types. Either they should be
cleaned out of the kernel headers by whatever's replacing make headers_install
for your toolchain/distro, or they should be #defined by those kernel headers
ala this chunk from 2.6.32's loop.h:
#include <asm/posix_types.h> /* for __kernel_old_dev_t */
#include <linux/types.h> /* for __u64 */
/* Backwards compatibility version */
struct loop_info {
int lo_number; /* ioctl r/o */
__kernel_old_dev_t lo_device; /* ioctl r/o */
Code that #includes linux/types.h shouldn't have to manually #define __u64. If
it does, its headers are broken. And if their linux/loop.h isn't #including
something to #define __u64, their linux/loop.h is broken. Variants of the same
conclusion.
In any case, if a horrible workaround like that was worth doing (which this
one isn't; they should fix their toolchain) it would belong in platform.h.
Making loop.c aware of the existence of specific compiler versions is kind of
evil. (It's bad enough making it aware of kernel versions, but that's really
an API test. Do we have the 64-bit API or not. It's possible that a cleaner
way to do that would be a "support old 2.4 kernel APIs" in menuconfig, but it
seems silly to ask people to manually select something we can autodetect at
compile time, which is why it was how it was, as the least ugly solution.)
All of that said, the git commit in question described itself as:
Patches to enable FreeBSD build
The notion of FreeBSD needing tweaks to linux/loop.h is absurd. This is a
Linux kernel API. If BSD is calling this at all, then they're explicitly
emulating the Linux kernel. (The name "linux" in the header path is a bit of
a hint here.) If they're not emulating the Linux kernel API correctly, this
is not our problem. It is their bug. They should either fix it, else or
acknowledge that they don't support this feature of the Linux kernel and
switch that config item off.
Rob
--
Latency is more important than throughput. It's that simple. - Linus Torvalds
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
