Re: [PATCH] selftests: clone3: Use the capget and capset syscall directly

[Shuah Khan]

On 10/15/24 03:00, zhouyuhang wrote:
>

[snip] for clarity.

> > > > > > Why is this necessary? This is defined in linux/capability.h.
>
> Sorry for not noticing this before.
> This is to be compatible with some older versions of linux/capability.h that do not define this macro.
>
> > > > > > > +int capget(cap_user_header_t header, cap_user_data_t data);
> > > > > > > +int capset(cap_user_header_t header, const cap_user_data_t data);
> > > > > >
> > > > > > In general prototypes such as these should be defined in header
> > > > > > file. Why are we defining these here?
> > > > > >
> > > > > > These are defined in sys/capability.h
> > > > > >
> > > > > > I don't understand this change. You are removing sys/capability.h
> > > > > > which requires you to add these defines here. This doesn't
> > > > > > sound like a correct solution to me.
> > > > >
> > > > > I tested it on my machine without libcap-dev installed, the /usr/include/linux/capability.h
> > > > >
> > > > > is on this machine by default. Successfully compiled using #include <linux/capability.h>
> > > > >
> > > > > but not with #include <sys/capability.h>. This patch removes libcap library dependencies.
> > > > >
> > > > > And we don't use any part of sys/capability.h other than these two syscalls. So I think that's why it's necessary.
> > > >
> > > > You are changing the code to not include sys/capability.h
> > > > What happens if sys/capability.h along with linux/capability.h
> > > >
> > > > Do you see problems?


> > > I'm sorry, maybe I wasn't clear enough.
> > > When we install the libcap library it will have the following output:
> > >
> > > test@test:~/work/libcap$ sudo make install | grep capability
> > > install -m 0644 include/sys/capability.h /usr/include/sys
> > > install -m 0644 include/sys/capability.h /usr/include/sys
> > > /usr/share/man/man5 capability.conf.5 \
> > >
> > > It installs sys/capability.h in the correct location, but does not
> > >
> > > install linux/capability.h, so sys/capability.h is bound to the libcap library
> >
> > It won't install inux/capability.h unless you run "make headers" in
> > the kernel repo.
> >
> > > and they will either exist or disappear together. Now I want to remove
> > >
> > > the dependency of the test on libcap library so I changed the code that it
> > >
> > > does not contain sys/capability.h but instead linux/capability.h,
> > >
> > > so that the test can compile successfully without libcap being installed,
> > >
> > > these two syscalls are not declared in linux/capability.h(It is sufficient for test use except for these two syscalls)
> > >
> > > so we need to declare them here. I think that's why the commit 663af70aabb7
> > >
> > > ("bpf: selftests: Add helpers to directly use the capget and capset syscall") I refered to
> > >
> > > does the same thing. As for your question "What happens if sys/capability.h along
> > >
> > > with linux/capability.h", I haven't found the problem yet, I sincerely hope you can
> > >
> > > help me improve this code. Thank you very much.
> >
> > Try this:
> >
> > Run make headers in the kernel repo.
> > Build without making any changes.
> > Then add you changes and add linux/capability.h to include files
> >
> > Tell me what happens.

Try the above first.

> > The change you are making isn't correct. Because you don't want to
> > define system calls locally in your source file.
>
> Thanks, I see.
> Maybe I should move them to a new header file and resend a patch.

No. Please see above. I would rather not see these defined at all
locally.

thanks,
-- Shuah