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

ll.h includes SquashFUSE's config.h #65

Open
reidpr opened this issue Aug 24, 2021 · 6 comments
Open

ll.h includes SquashFUSE's config.h #65

reidpr opened this issue Aug 24, 2021 · 6 comments

Comments

@reidpr
Copy link

reidpr commented Aug 24, 2021

Thanks for the library support (PR #59); this helps us a lot!!

One challenge we ran into is that ll.h includes (via some intermediate headers IIRC) the Autoconf-produced config.h. This clashes with two things:

  1. our own Autoconf-produced config.h, and
  2. something in the system headers, because it defines _POSIX_C_SOURCE to a different value than our project.

We did find a workaround:

#define SQFS_CONFIG_H
#define FUSE_USE_VERSION 32
#include <squashfuse/ll.h>

(Note that we are libfuse3 only so that's why it seemed OK to define FUSE_USE_VERSION unconditionally.)

But it would be nice if there were an externally-facing header to include.

@DrDaveD
Copy link
Collaborator

DrDaveD commented Jun 28, 2023

Can you submit a PR with a suggested fix?

@reidpr
Copy link
Author

reidpr commented Jul 23, 2023

We don’t have the expertise on this project to submit a PR; sorry.

@DrDaveD
Copy link
Collaborator

DrDaveD commented Jul 23, 2023

I don't really understand how your workaround works or what it does. I don't see how it can get away with ignoring all the config.h definitions determined by configure.

@reidpr
Copy link
Author

reidpr commented Jul 23, 2023

config.h is an internal header and shouldn't be exposed to other programs using the library. Thus, it was fine to have it included by ll.h when there was no shared library, but now that there is, including that file may break any library user that also uses Autoconf.

The workaround above works by preventing ll.h from defining the specific symbols that clash with Charliecloud's build. It is not a general fix. The general fix would be to separate the SquashFUSE headers into internal and external (which requires knowledge of SquashFUSE we don't have).

@DrDaveD
Copy link
Collaborator

DrDaveD commented Jul 24, 2023

I see, that makes sense. Perhaps the developer of the library interface @haampie could come up with a PR for that?

@vasi
Copy link
Owner

vasi commented Aug 5, 2023

Here are the config.h-related things in squashfuse headers:

  • squashfs_fs.h: SQFS_MULTITHREADED. We only use this to decide the number of cached blocks, which is only for internal use. We should move that to the private header.
  • squashfs_fs.h: HAVE_LINUX_TYPES_LE16. The <stdint.h> replacement for this should always work, maybe we should just do that unconditionally?
  • ll.h: FUSE_XATTR_POSITION and FUSE_USE_VERSION (for the extra fuse_chan field in sqfs_ll_chan. These are really bad, because they're API changes based on configure! If you compile against squashfuse, and then later squashfuse is rebuild with different configure args, these could theoretically break. But honestly, they're both internals that never should have been exposed in a public header--we probably need to figure out what is really internal vs. external, and keep the internals private.

@vasi vasi mentioned this issue Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants