Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LFS, Large File Support, can it be dropped? #445

Closed
Dead2 opened this issue Oct 7, 2019 · 12 comments
Closed

LFS, Large File Support, can it be dropped? #445

Dead2 opened this issue Oct 7, 2019 · 12 comments
Labels
cleanup Improving maintainability or removing code. discussion Investigation needed

Comments

@Dead2
Copy link
Member

Dead2 commented Oct 7, 2019

I am considering whether we should drop support for LFS in zlib-ng's native ABI and possibly partially in the compat code as well.

Assume we always define: _FILE_OFFSET_BITS=64?, that should replace all the LFS-affected libc functions with 64-bit capable ones (without requiring us to call the *64 variants).

Assuming we always have 64-bit file-functions and a off64_t data-type could simplify some parts of the zlib-ng native ABI, and it would be great to drop that legacy stuff before a stable release if possible. I am pretty sure we have at least 2 bugs and probably more in the current LFS support because we likely never test on a platform that lacks this support.

The question is, what platforms exist where this would not work? Any at all?

I have been unable to find any kind of table showing where this is supported/required and not, and the only platform I can think of that most likely don't support it would be Arduino-class 8bit stuff, but using zlib-ng there would be crazy anyhow.

In any case we can drop support for the 32-bit functions from our exported ABI and enforce the use of 64-bit parameters (and without using the 64-suffix for the function names). Worst case, this would mean false-advertising of 64bit functions if the underlying libc does not support it, but in that case it is probably an OS-wide issue and not really a problem.

Zlib-ng native ABI:

  • Only export 64-bit functions (but drop the 64-suffix)
  • Always define _FILE_OFFSET_BITS=64 when compiling on *nix
  • Drop LFS-special handling

Zlib-ng zlib-compat ABI:
We could possibly drop support LFS-special handling and define _FILE_OFFSET_BITS=64. We must still export the legacy 32-bit functions, but internally they can be 64bit. This is the same as the _FILE_OFFSET_BITS=64 case in fact, and is likely what always occurs on 64-bit platforms anyhow. Doing the same for 32bit platforms should probably not be a problem any more, if we require that the libc in use supports the 64bit functions that is.

  • Export 32-bit and 64-bit functions (internally they are both 64bit)
  • Always define _FILE_OFFSET_BITS=64 when compiling on *nix
  • Drop LFS-special handling
@Dead2 Dead2 added discussion Investigation needed cleanup Improving maintainability or removing code. labels Oct 7, 2019
@nmoinvaz
Copy link
Member

nmoinvaz commented Oct 7, 2019

I have encountered some people on Android mention it not supporting LFS:
zlib-ng/minizip-ng#221 and zlib-ng/minizip-ng#138

This is what I did for minizip:
https://github.com/nmoinvaz/minizip/blob/6f9a9c6b1117def0c88944b607c3cd8251d68729/mz_strm_os_posix.c#L27

open and close functions do not need 64-bit versions. So instead of making my own 64-bit compatible functions seekhere() and seekhere64(). I just have one seekhere() that takes 64-bit and underneath it calls the correct function for the platform.

I removed stuff like this found in gzguts.h which was also in ioapi.h in minizip (but worse):

#ifdef _LARGEFILE64_SOURCE
#  ifndef _LARGEFILE_SOURCE
#    define _LARGEFILE_SOURCE 1
#  endif
#  ifdef _FILE_OFFSET_BITS
#    undef _FILE_OFFSET_BITS
#  endif
#endif

And my CMakeLists.txt just defines all necessary #defines. I have a single MZ_FILE32_API which determines whether or not it should use the 32-bit functions but the public interface always supports 64-bit fixed types.

@Dead2
Copy link
Member Author

Dead2 commented Oct 7, 2019

One of the sources I used when looking at this is the second answer here: https://stackoverflow.com/questions/9073667/where-to-find-the-complete-definition-of-off-t-type
Especially points 2 and 3 are interesting there.

2. As was mentioned before compile your code with
   -D_FILE_OFFSET_BITS == 64 and use usual off_t and stat().
3. Convert off_t to type int64_t with fixed size (C99 standard).
   Note: (my book 'C in a Nutshell' says that it is C99 standard,
   but optional in implementation). The newest C11 standard says:

Since off_t and off64_t are posix and not in C99, it would make sense to changing to using int64_t instead, at least for the zlib-ng native ABI. That is another cludge (see zconf.h) that we could avoid.

Feel free to pitch in your thoughts on the matter.
If I don't see anything that convinces me that this is a bad idea, I'll make a PR for this soon(TM).

@Dead2
Copy link
Member Author

Dead2 commented Oct 7, 2019

@nmoinvaz Interesting. Android API version 23 (Andoid 6) is no longer supported by google, so I guess that might no longer be so critical.
I guess I would be OK with dropping support for deprecated Android versions if it gives us a cleaner and more straight-forward ABI.

@nmoinvaz
Copy link
Member

nmoinvaz commented Oct 7, 2019

I think it should be possible to continue to provide compiler support for 32-bit file api while making our interface 64-bit only.

@nmoinvaz
Copy link
Member

nmoinvaz commented May 27, 2020

For zlib-ng native API if we export 64-bit functions without "64" ending, how do we also export 32-bit and 64-bit functions for zlib compatible API?

Zlib-ng native API would export zng_gzseek which would be 64-bit version.
Zlib compatible API would export zng_gzseek which would be 32-bit version.

So there is a naming conflict.

Only way would be in zlib-ng native API #define zng_gzseek zng_gzseek64. But that doesn't seem to be much of a benefit or am I wrong? That seems to be the same as defining Z_WANT64.

@mtl1979
Copy link
Collaborator

mtl1979 commented May 27, 2020

Obviously zlib compatible API would export the 32-bit version as "gzseek" without the prefix.

@Dead2
Copy link
Member Author

Dead2 commented May 28, 2020

mtl1979 is right.
But in the above example, if we needed to we could have had a shim named gzseek32 and sent the no-postfix define there.

@mtl1979
Copy link
Collaborator

mtl1979 commented May 28, 2020

Only when we dual link both stock zlib and zlib-ng in compatibility mode, we need compatibility mode functions to have prefix, but the default is that compatibility mode does not use prefix.

@Dead2
Copy link
Member Author

Dead2 commented May 28, 2020

Yeah, compat mode should not use a prefix. Dual linking in compat mode is not something we plan to support, only for the zlib-ng native api.

@mtl1979
Copy link
Collaborator

mtl1979 commented May 28, 2020

Dual linking stock zlib and zlib-ng might be more likely to happen unintentionally when linking against another library that contains zlib as dependency.

@Dead2
Copy link
Member Author

Dead2 commented May 28, 2020

Yes, that is why people should use zlib-ng for that instead.
zlib-ng in compat mode is more useful for systemwide replacement, where it will not conflict with another zlib library.

@nmoinvaz
Copy link
Member

nmoinvaz commented Jun 8, 2020

I believe this work has been done in #602. If there is work left I can reopen.

@nmoinvaz nmoinvaz closed this as completed Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Improving maintainability or removing code. discussion Investigation needed
Projects
None yet
Development

No branches or pull requests

3 participants