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

Fix compatibility issue around fuse_custom_io->clone_fd #953

Merged
merged 1 commit into from
Jun 1, 2024

Conversation

legezywzh
Copy link
Contributor

Fixes: 73cd124 ("Add clone_fd to custom IO (#927)")

#define _fuse_session_custom_io(se, io, op_size, fd) \
_fuse_session_custom_io_317(se, io, op_size, fd)
#endif
#define fuse_session_custom_io(se, io, fd) \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this macro? I had introduced the inlined function fuse_session_new which calls into _fuse_session_new to encode in the libfuse version an application was compiled against. Maybe I should also do that for fuse_session_custom_io(). But right now this macro here does not bring an advantage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this macro? I had introduced the inlined function fuse_session_new which calls into _fuse_session_new to encode in the libfuse version an application was compiled against. Maybe I should also do that for fuse_session_custom_io(). But right now this macro here does not bring an advantage?

Do you mean change to below:

@@ -2119,8 +2130,13 @@ fuse_session_new(struct fuse_args *args,
  * @return -errno  if failed to allocate memory to store `io`
  *
  **/ 
+#if (defined(LIBFUSE_BUILT_WITH_VERSIONED_SYMBOLS))
 int fuse_session_custom_io(struct fuse_session *se,
                   const struct fuse_custom_io *io, int fd);
+#else
+#define fuse_session_custom_io(se, io, fd) \
+   _fuse_session_custom_io_317(se, io, sizeof(*io), fd)
+#endif

But original fuse_session_custom_io() API doesn't support to pass sizeof(fuse_custom_io ) info, so for LIBFUSE_BUILT_WITH_VERSIONED_SYMBOLS branch, it won't work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only see

#if  (defined(LIBFUSE_BUILT_WITH_VERSIONED_SYMBOLS))
int _fuse_session_custom_io(struct fuse_session *se,...
...
#endif
#define fuse_session_custom_io(se, io, fd) _fuse_session_custom_io(se, io, sizeof(*io), fd)

I.e. redirection of fuse_session_custom_io to the underscored function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think now I got what you mean.
Indeed we can change original fuse_session_custom_io() API to add sizeof(fuse_custom_io ) info for LIBFUSE_BUILT_WITH_VERSIONED_SYMBOLS branch, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, seems that I was wrong, API must not be changed, Could you please give some code hints.

@bsbernd
Copy link
Collaborator

bsbernd commented May 27, 2024

@legezywzh Thanks for working on this, looks good to me, just a comment about the "_" name

@bsbernd
Copy link
Collaborator

bsbernd commented May 27, 2024

Ah right, what about making it an inlined function instead of a macro?

int fuse_session_custom_io_317(struct fuse_session *se, const struct fuse_custom_io *io, size_t op_size, int fd);
#if FUSE_USE_VERSION < FUSE_MAKE_VERSION(3, 17)
static inline int fuse_session_custom_io(struct fuse_session *se, const struct fuse_custom_io *io, size_t op_size, int fd)
{
     return fuse_session_custom_io_317(se, io, op_size, fd);
}
#else
static inline int fuse_session_custom_io(struct fuse_session *se, const struct fuse_custom_io *io, int fd)
{
   return fuse_session_custom_io_317(se, io, offsetof(struct fuse_custom_io, clone_fd), fd);
}
#endif

There is also no underscore "_" and version check is "<" instead of "<=" as 3.17 is not released yet.

@bsbernd
Copy link
Collaborator

bsbernd commented May 27, 2024

(personally I prefer inlined functions over macros, though just my personal opinion)

@legezywzh
Copy link
Contributor Author

Ah right, what about making it an inlined function instead of a macro?

int fuse_session_custom_io_317(struct fuse_session *se, const struct fuse_custom_io *io, size_t op_size, int fd);
#if FUSE_USE_VERSION < FUSE_MAKE_VERSION(3, 17)
static inline int fuse_session_custom_io(struct fuse_session *se, const struct fuse_custom_io *io, size_t op_size, int fd)
{
     return fuse_session_custom_io_317(se, io, op_size, fd);
}
#else
static inline int fuse_session_custom_io(struct fuse_session *se, const struct fuse_custom_io *io, int fd)
{
   return fuse_session_custom_io_317(se, io, offsetof(struct fuse_custom_io, clone_fd), fd);
}
#endif

There is also no underscore "_" and version check is "<" instead of "<=" as 3.17 is not released yet.

then there'll be compile error:
../lib/compat.c:59:5: error: redefinition of ‘fuse_session_custom_io’
59 | int fuse_session_custom_io(struct fuse_session *se,
| ^~~~~~~~~~~~~~~~~~~~~~

See patch codes about compat.c

@legezywzh
Copy link
Contributor Author

Ah right, what about making it an inlined function instead of a macro?

int fuse_session_custom_io_317(struct fuse_session *se, const struct fuse_custom_io *io, size_t op_size, int fd);
#if FUSE_USE_VERSION < FUSE_MAKE_VERSION(3, 17)
static inline int fuse_session_custom_io(struct fuse_session *se, const struct fuse_custom_io *io, size_t op_size, int fd)
{
     return fuse_session_custom_io_317(se, io, op_size, fd);
}
#else
static inline int fuse_session_custom_io(struct fuse_session *se, const struct fuse_custom_io *io, int fd)
{
   return fuse_session_custom_io_317(se, io, offsetof(struct fuse_custom_io, clone_fd), fd);
}
#endif

There is also no underscore "_" and version check is "<" instead of "<=" as 3.17 is not released yet.

Or do you mean that using this code snippet, we don't need to do versioned symbols check about fuse_session_custom_io()?

@bsbernd
Copy link
Collaborator

bsbernd commented May 27, 2024

We need versioned symbols for ABI, but for API I thought this code snippet would do.

Regarding compat.c, this PR should fix your issue
#954

@bsbernd
Copy link
Collaborator

bsbernd commented May 27, 2024

I merged PR #954, you shouldn't get the compilation error after rebase.

Fixes: 73cd124 ("Add clone_fd to custom IO (libfuse#927)")
Signed-off-by: Xiaoguang Wang <lege.wang@jaguarmicro.com>
@legezywzh
Copy link
Contributor Author

@bsbernd I have updated one new version, thanks for for your patience.

And I have one personal question, in the development of libfuse, API can be changed in the new libfuse version, right? It only requires that old ABI is still provided, and userspace filesystems codes need to adapt new API and re-compile.
Do I understand correctly?

@bsbernd
Copy link
Collaborator

bsbernd commented May 28, 2024

Sorry I didn't manage to look into this today (actually already past midnight for me now), will respond after getting some sleep.

@legezywzh
Copy link
Contributor Author

Sorry I didn't manage to look into this today (actually already past midnight for me now), will respond after getting some sleep.

Really never mind, just take your time.

@bsbernd bsbernd merged commit 949944f into libfuse:master Jun 1, 2024
2 checks passed
@bsbernd
Copy link
Collaborator

bsbernd commented Jun 1, 2024

@legezywzh Thank you, also for the updates! LGTM, merged.

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

Successfully merging this pull request may close these issues.

None yet

2 participants