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

allow compile without FS_IOC_FSGETXATTR #3602

Closed
wants to merge 1 commit into from

Conversation

GitMensch
Copy link
Contributor

follow-up to a2d76a6

@rocallahan
Copy link
Collaborator

A better fix would be to conditionally define FS_IOC_FSGETXATTR in kernel_supplement.h and in the test.

We should really make kernel_supplement.h usable from tests so these only need to be added in one place.

@GitMensch
Copy link
Contributor Author

We should really make kernel_supplement.h usable from tests so these only need to be added in one place.

Is this something I should give a try? What needs to be done (=why isn't it usable)?

@rocallahan
Copy link
Collaborator

Is this something I should give a try?

Sure.

What needs to be done (=why isn't it usable)?

The problem is that kernel_supplement.h uses values from kernel_abi.h which is heavily C++ dependent and the tests are in C. I think we probably need to extract the C-compatible stuff from kernel_supplement.h into a new header file, say kernel_supplement_c.h that can be included by kernel_supplement.h and from test/util.h. kernel_supplement_c.h will need to #include the right Linux header files.

@GitMensch
Copy link
Contributor Author

Hm, what about FS_IOC_FSGETXATTRA and FS_IOC_FSSETXATTR? Don't we need an explicit support/test for that?

In any case, that's what we may want to add to a supplemental header:

#ifndef FS_IOC_FSGETXATTR
/*
 * Structure for FS_IOC_FSGETXATTR[A] and FS_IOC_FSSETXATTR.
 */
struct fsxattr {
	__u32		fsx_xflags;	/* xflags field value (get/set) */
	__u32		fsx_extsize;	/* extsize field value (get/set)*/
	__u32		fsx_nextents;	/* nextents field value (get)	*/
	__u32		fsx_projid;	/* project identifier (get/set) */
	__u32		fsx_cowextsize;	/* CoW extsize field value (get/set)*/
	unsigned char	fsx_pad[8];
};
#define FS_IOC_FSGETXATTR		_IOR('X', 31, struct fsxattr)
#define FS_IOC_FSSETXATTR		_IOW('X', 32, struct fsxattr)
#endif

//#ifndef XFS_IOC_FSGETXATTR
//#define XFS_IOC_FSGETXATTR	FS_IOC_FSGETXATTR
//#define XFS_IOC_FSSETXATTR	FS_IOC_FSSETXATTR
//#endif
#ifndef XFS_IOC_FSGETXATTRA
#define XFS_IOC_FSGETXATTRA	_IOR ('X', 45, struct fsxattr)
#endif

The XFS_IOC_FSGETXATTR block isn't needed as that was available before FS_IOC_FSGETXATTR got moved out of the XFS interface.

... but checking that: it seems that this was done with kernel version 4.5 and as rr now requests 4.7, I'd close this issue by adding this as necessary piece to README.md and consider it done.
The only thing that is then open:

what about FS_IOC_FSGETXATTRA and FS_IOC_FSSETXATTR? Don't we need an explicit support/test for that?

(that's above my knowledge)

Moving the supplement out is likely a reasonable thing (but definitely more work),

@GitMensch GitMensch mentioned this pull request Oct 4, 2023
GitMensch added a commit to GitMensch/rr that referenced this pull request Oct 4, 2023
we need `FS_IOC_FSSETXATTR`;
it is one of the reasons to require a newer kernel for newer versions of rr

technical note: that arrived with kernel 4.5...

closes rr-debugger#3602
@GitMensch
Copy link
Contributor Author

@rocallahan I'd suggest to close this PR as we found that does not apply to RR any more because of the higher kernel dependency.

about FS_IOC_FSGETXATTRA and FS_IOC_FSSETXATTR? Don't we need an explicit support/test for that?

-> I'm leaving this decision to you, it is not directly related to this PR either.

@rocallahan
Copy link
Collaborator

ok thanks

@rocallahan rocallahan closed this Oct 4, 2023
@GitMensch GitMensch deleted the FSGETXATTR branch October 4, 2023 11:58
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.

2 participants