Bug 4053 - off_t size in libsmbclient.h depends on LFS defines
off_t size in libsmbclient.h depends on LFS defines
Status: ASSIGNED
Product: Samba 3.0
Classification: Unclassified
Component: libsmbclient
3.0.23b
x86 Linux
: P3 minor
: none
Assigned To: Derrell Lipman
Samba QA Contact
http://bugs.debian.org/cgi-bin/bugrep...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-08-26 02:07 UTC by Peter Eisentraut
Modified: 2007-06-19 10:42 UTC (History)
2 users (show)

See Also:


Attachments
Proper LFS support (194.95 KB, patch)
2007-01-14 18:58 UTC, Samuel Thibault
no flags Details
restricted LFS support (174.58 KB, patch)
2007-01-14 19:02 UTC, Samuel Thibault
no flags Details
Proper LFS support for svn (194.95 KB, patch)
2007-05-16 20:05 UTC, Samuel Thibault
no flags Details
Proper LFS support for svn (194.92 KB, patch)
2007-05-16 20:40 UTC, Samuel Thibault
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Eisentraut 2006-08-26 02:07:17 UTC
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.
Comment 1 Jeremy Allison 2006-08-26 02:12:16 UTC
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.
Comment 2 Derrell Lipman 2006-09-02 20:41:18 UTC
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?
Comment 3 Samuel Thibault 2006-10-28 17:15:51 UTC
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...
Comment 4 Derrell Lipman 2006-10-28 18:54:18 UTC
If you have any suggestions to fix it that maintain binary backward compoatibility, I'd be happy to entertain them.
Comment 5 Samuel Thibault 2006-10-28 19:06:37 UTC
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. 
Comment 6 Derrell Lipman 2006-10-28 20:24:00 UTC
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. :-)
Comment 7 Walter Doekes 2006-11-26 10:09:50 UTC
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.
Comment 8 Samuel Thibault 2007-01-14 18:58:01 UTC
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.
Comment 9 Samuel Thibault 2007-01-14 19:02:09 UTC
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...)
Comment 10 Jeremy Allison 2007-04-16 17:49:12 UTC
Derrell, did you ever evaluate these ? The full LFS support looks interesting. I might add this in for 3.0.26.
Jeremy.
Comment 11 Derrell Lipman 2007-04-16 20:51:30 UTC
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!
Comment 12 Spammer 2007-04-28 04:22:51 UTC
Created attachment 2502
Comment 13 Derrell Lipman 2007-05-16 09:15:01 UTC
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
Comment 14 Samuel Thibault 2007-05-16 19:12:50 UTC
(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.
Comment 15 Samuel Thibault 2007-05-16 20:05:17 UTC
Created attachment 2698 [details]
Proper LFS support for svn

Fixes the #defines
Comment 16 Jeremy Allison 2007-05-16 20:14:53 UTC
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.
Comment 17 Samuel Thibault 2007-05-16 20:40:21 UTC
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 18 Samuel Thibault 2007-05-16 21:26:03 UTC
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
Comment 19 Samuel Thibault 2007-05-16 21:45:35 UTC
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).