libsmbclient.h exposes function prototypes using the off_t type. The size of this type varies depending on whether -D_FILE_OFFSET_BITS=64 (or local equivalent) is used. The actual library, however, is already compiled one way or the other, but the user doesn't know that, so there's a 50/50 chance to create a type mismatch when using these functions. So either the relevant defines must be hacked into libsmbclient.h during the build or otherwise made available at run time. Or perhaps it's easier to not use off_t at all. (There may be other instances besides off_t.) Or at least this needs to be very carefully documented. With large-file support becoming the standard, a lot of people are prone to get this wrong.
I just ran into this inside SuSE so this bug is very well timed :-). Unfortunately, I think documenting it is all we can do, as we expose 'struct stat' from the library, whose size changes depending on whether we used LFS or not (that's what I ran into internally). Jeremy.
This is a generic problem with any library that returns types that vary in size based on compilation settings. Along with documentation, we could also provide a function that returned a structure or dictionary containing the sizes of various types, as determined at the time of compilation of the library. Would this be useful?
Note: documenting is not really enough, in case a program wants to use both a library using off_t and compiled without LFS support, and a library using off_t and compiled _with_ LFS support. Both libraries can't be used at the same time...
If you have any suggestions to fix it that maintain binary backward compoatibility, I'd be happy to entertain them.
Well, the only solution I can see is to compile functions twice, once without LFS defines and once with LFS defines. Then, in libsmbclient.h, you can #define smbc_lseek to either, according to the value of LFS defines. Same for stat, etc.
Interesting option. What gets difficult, though, is dealing with all of the ifdefs required to _not_ compile code if the _64 fuunctions don't exist, if you want to reference the 64-bit functions explicitely. Since the distros need to compile this, I'm really more inclined to leave it to the distros to compile with -D_FILE_OFFSET_BITS=64 if they intend for most things to be using the 64-bit interface, and without it otherwise. The added complexity within libsmbclient just doesn't seem to be worth it. (Only two people have ever mentioned this as an issue.) I will consider applying a nice clean patch, if you'd like to submit one. :-)
I would've been perfectly content with something like: $ diff -uw libsmbclient.h.orig libsmbclient.h --- libsmbclient.h.orig 2006-11-26 17:02:31.000000000 +0100 +++ libsmbclient.h 2006-11-26 17:01:45.000000000 +0100 @@ -67,6 +67,11 @@ /*-------------------------------------------------------------------*/ /* Make sure we have the following includes for now ... */ +#if defined(_MSC_VER) /* visual studio cpp does not know #warning */ + /*# pragma warning Optional blah, if we know how to check for 64 bits off_t here. */ +#elif _FILE_OFFSET_BITS != 64 /* assume other preprocessors know #warning */ +# warning Please define _FILE_OFFSET_BITS as 64 to get stat working correctly. +#endif #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> That would've saved me 1.5hrs of my time, as I couldn't understand why smbc_stat was overwriting my url/fname :-) With this you'd run into the same problem with distros compiling without _FILE_OFFSET_BITS=64 unfortunately, but this seems like less of a problem as most are using the 64-bit interface.
Created attachment 2255 [details] Proper LFS support This patch adds full LFS support: - it uses the existing members in the SMBCCTX structure for legacy non-LFS methods, and adds new members for the LFS methods, - it keeps the existing context-less functions for legacy non-LFS applications, and adds -64 context-less functions for LFS applications, macros make it so that both the source code and the application code choose the right methods/functions to use. Note error handling in libsmb_nonfs.c: if values don't fit, EOVERFLOW is returned, as specified by LFS.
Created attachment 2256 [details] restricted LFS support This patch implements a much less satisfactory LFS support than the previous one, but it is much less instrusive: it just detects when the user tries to compile with LFS while libsmbclient wasn't compiled with LFS, and vice-versa (#error may seem harsh, but lseek/stat will just _not_ work, so...)
Derrell, did you ever evaluate these ? The full LFS support looks interesting. I might add this in for 3.0.26. Jeremy.
Jeremy, I looked at it and decided that the test infrastructure to ensure it worked properly (and didn't break backward compatibility) was going to a fair amount of work. I have this on my list of things to do, and the concept seemed fine, but I hadn't yet been able to allocate the time I thought was necessary to be able to test it properly. I'd certainly welcome this being incorporated, with an appropriate test structure for it!
Created attachment 2502
Samuel, thanks for submitting the LFS patch! I'd like to apply a patch like this, and I just reviewed yours in more detail. It generally looks quite reasonable. I still need to confirm that it does not break backwards compatibility. I noticed a couple of things that don't make sense to me, so I hope you can explain. 1. In configure.in, you set SMBC_FILE_OFFSET_BITS but then in libsmbclient.h.in you explicitly #undef it immediately before testing to see if it is defined. Was this just for testing? 2. For each function that you create 64-bit verions, you have code like this: +off_t smbc_lseek(int fd, off_t offset, int whence); +#if defined(SMBC_FILE_OFFSET_BITS) && SMBC_FILE_OFFSET_BITS == 64 && \ + defined(_FILE_OFFSET_BITS) && _FILE_OFFSET_BITS == 64 +off_t smbc_lseek64(int fd, off_t offset, int whence); +#define smbc_lseek smbc_lseek64 +#endif but every other function, e.g. smbc_telldir, also defines smbc_lseek (not, in this case smbc_telldir): +off_t smbc_telldir(int dh); +#if defined(SMBC_FILE_OFFSET_BITS) && SMBC_FILE_OFFSET_BITS == 64 && \ + defined(_FILE_OFFSET_BITS) && _FILE_OFFSET_BITS == 64 +off_t smbc_telldir64(int dh); +#define smbc_lseek smbc_lseek64 +#endif Presumably these are copy/paste errors. Have you been using an LFS patch? Do you have an updated version that is (at this point) well tested? Thanks! Derrell
(In reply to comment #13) > 1. In configure.in, you set SMBC_FILE_OFFSET_BITS but then in libsmbclient.h.in > you explicitly #undef it immediately before testing to see if it is defined. > Was this just for testing? No, that's how .in files work: you #undef SYMBOL and in configure.in you AC_DEFINE(SYMBOL,foo), ./configure replaces #undef SYMBOL by #define SYMBOL foo > 2. For each function that you create 64-bit verions, you have code like this: > +off_t smbc_lseek(int fd, off_t offset, int whence); > +#if defined(SMBC_FILE_OFFSET_BITS) && SMBC_FILE_OFFSET_BITS == 64 && \ > + defined(_FILE_OFFSET_BITS) && _FILE_OFFSET_BITS == 64 > +off_t smbc_lseek64(int fd, off_t offset, int whence); > +#define smbc_lseek smbc_lseek64 > +#endif > > but every other function, e.g. smbc_telldir, also defines smbc_lseek (not, in > this case smbc_telldir): > Presumably these are copy/paste errors. Eeergl, yes, that's it. > Have you been using an LFS patch? Do you have an updated version that is (at > this point) well tested? Nope, I only tested smbc_lseek(). I actually don't really use libsmbclient myself, some friends asked me whether I could fix that for them.
Created attachment 2698 [details] Proper LFS support for svn Fixes the #defines
One problem I can see with the patch is that you're using typeof(), which is a gcc-specific extension. Can we replace this with something portable ? Jeremy.
Created attachment 2699 [details] Proper LFS support for svn Ah, yes. Well, actually the cast is not needed: the implicit promotion is correct, here is a fixed patch.
Comment on attachment 2699 [details] Proper LFS support for svn BTW, smbc_new_legacy_context should be declared somewhere, I don't know what place would be best
Ok, I tested lseek again, as well as stat, and also fstat through smbc, works as expected (i.e. getting error EOVERFLOW for big files, unless compiled with _FILE_OFFSET_BITS equal to 64).