diff mbox series

[v3,1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap

Message ID 1649763226-2329-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/5] idmapped-mounts: Reset errno to zero after detect fs_allow_idmap | expand

Commit Message

Yang Xu (Fujitsu) April 12, 2022, 11:33 a.m. UTC
If we run case on old kernel that doesn't support mount_setattr and
then fail on our own function before call is_setgid/is_setuid function
to reset errno, run_test will print "Function not implement" error.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 src/idmapped-mounts/idmapped-mounts.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Christian Brauner April 13, 2022, 7:50 a.m. UTC | #1
On Tue, Apr 12, 2022 at 07:33:42PM +0800, Yang Xu wrote:
> If we run case on old kernel that doesn't support mount_setattr and
> then fail on our own function before call is_setgid/is_setuid function
> to reset errno, run_test will print "Function not implement" error.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

Looks good to me,
Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Yang Xu (Fujitsu) May 7, 2022, 1:33 a.m. UTC | #2
Hi Zorro

Since  Christian doesn't send  a new patchset(for rename idmap-mount)
based on lastest xfstests, should I send a v4 patch for the following
patches today?
"idmapped-mounts: Reset errno to zero after detect fs_allow_idmap"
" idmapped-mounts: Add mknodat operation in setgid test"
"idmapped-mounts: Add open with O_TMPFILE operation in setgid test"

So you can merge these three patches if you plan to announce a new
xfstests version in this weekend.

What do you think about it?

Best Regards
Yang Xu
> If we run case on old kernel that doesn't support mount_setattr and
> then fail on our own function before call is_setgid/is_setuid function
> to reset errno, run_test will print "Function not implement" error.
> 
> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> ---
>   src/idmapped-mounts/idmapped-mounts.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> index 4cf6c3bb..8e6405c5 100644
> --- a/src/idmapped-mounts/idmapped-mounts.c
> +++ b/src/idmapped-mounts/idmapped-mounts.c
> @@ -14070,6 +14070,8 @@ int main(int argc, char *argv[])
>   		die("failed to open %s", t_mountpoint_scratch);
> 
>   	t_fs_allow_idmap = fs_allow_idmap();
> +	/* don't copy ENOSYS errno to child process on older kernel */
> +	errno = 0;
>   	if (supported) {
>   		/*
>   		 * Caller just wants to know whether the filesystem we're on
Zorro Lang May 7, 2022, 8:52 a.m. UTC | #3
On Sat, May 07, 2022 at 01:33:33AM +0000, xuyang2018.jy@fujitsu.com wrote:
> Hi Zorro
> 
> Since  Christian doesn't send  a new patchset(for rename idmap-mount)
> based on lastest xfstests, should I send a v4 patch for the following
> patches today?
> "idmapped-mounts: Reset errno to zero after detect fs_allow_idmap"
> " idmapped-mounts: Add mknodat operation in setgid test"
> "idmapped-mounts: Add open with O_TMPFILE operation in setgid test"
> 
> So you can merge these three patches if you plan to announce a new
> xfstests version in this weekend.
> 
> What do you think about it?

Sure, you can send V4 of patch 1/5 ~ 3/5 (base on latest for-next branch
please), as they have been reviewed and tested. Christian's patch (about
refactor idmapped testing) might need more review, he just sent it out to
get some review points I think (cc Christian).

If you'd like to catch up the release of this weekend, please send your
v4 patch ASAP. Due to I need time to do regression test before pushing.
It'll wait for next week if too late.

Thanks,
Zorro

> 
> Best Regards
> Yang Xu
> > If we run case on old kernel that doesn't support mount_setattr and
> > then fail on our own function before call is_setgid/is_setuid function
> > to reset errno, run_test will print "Function not implement" error.
> > 
> > Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> > ---
> >   src/idmapped-mounts/idmapped-mounts.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
> > index 4cf6c3bb..8e6405c5 100644
> > --- a/src/idmapped-mounts/idmapped-mounts.c
> > +++ b/src/idmapped-mounts/idmapped-mounts.c
> > @@ -14070,6 +14070,8 @@ int main(int argc, char *argv[])
> >   		die("failed to open %s", t_mountpoint_scratch);
> > 
> >   	t_fs_allow_idmap = fs_allow_idmap();
> > +	/* don't copy ENOSYS errno to child process on older kernel */
> > +	errno = 0;
> >   	if (supported) {
> >   		/*
> >   		 * Caller just wants to know whether the filesystem we're on
Yang Xu (Fujitsu) May 7, 2022, 9:12 a.m. UTC | #4
on 2022/5/7 16:52, Zorro Lang wrote:
> On Sat, May 07, 2022 at 01:33:33AM +0000, xuyang2018.jy@fujitsu.com wrote:
>> Hi Zorro
>>
>> Since  Christian doesn't send  a new patchset(for rename idmap-mount)
>> based on lastest xfstests, should I send a v4 patch for the following
>> patches today?
>> "idmapped-mounts: Reset errno to zero after detect fs_allow_idmap"
>> " idmapped-mounts: Add mknodat operation in setgid test"
>> "idmapped-mounts: Add open with O_TMPFILE operation in setgid test"
>>
>> So you can merge these three patches if you plan to announce a new
>> xfstests version in this weekend.
>>
>> What do you think about it?
>
> Sure, you can send V4 of patch 1/5 ~ 3/5 (base on latest for-next branch
> please), as they have been reviewed and tested. Christian's patch (about
> refactor idmapped testing) might need more review, he just sent it out to
> get some review points I think (cc Christian).
>
> If you'd like to catch up the release of this weekend, please send your
> v4 patch ASAP. Due to I need time to do regression test before pushing.
> It'll wait for next week if too late.

OK. I will send v4 patch ASAP today.

>
> Thanks,
> Zorro
>
>>
>> Best Regards
>> Yang Xu
>>> If we run case on old kernel that doesn't support mount_setattr and
>>> then fail on our own function before call is_setgid/is_setuid function
>>> to reset errno, run_test will print "Function not implement" error.
>>>
>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>> ---
>>>    src/idmapped-mounts/idmapped-mounts.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
>>> index 4cf6c3bb..8e6405c5 100644
>>> --- a/src/idmapped-mounts/idmapped-mounts.c
>>> +++ b/src/idmapped-mounts/idmapped-mounts.c
>>> @@ -14070,6 +14070,8 @@ int main(int argc, char *argv[])
>>>    		die("failed to open %s", t_mountpoint_scratch);
>>>
>>>    	t_fs_allow_idmap = fs_allow_idmap();
>>> +	/* don't copy ENOSYS errno to child process on older kernel */
>>> +	errno = 0;
>>>    	if (supported) {
>>>    		/*
>>>    		 * Caller just wants to know whether the filesystem we're on
>
Christian Brauner May 7, 2022, 11:40 a.m. UTC | #5
On Sat, May 07, 2022 at 04:52:09PM +0800, Zorro Lang wrote:
> On Sat, May 07, 2022 at 01:33:33AM +0000, xuyang2018.jy@fujitsu.com wrote:
> > Hi Zorro
> > 
> > Since  Christian doesn't send  a new patchset(for rename idmap-mount)
> > based on lastest xfstests, should I send a v4 patch for the following
> > patches today?
> > "idmapped-mounts: Reset errno to zero after detect fs_allow_idmap"
> > " idmapped-mounts: Add mknodat operation in setgid test"
> > "idmapped-mounts: Add open with O_TMPFILE operation in setgid test"
> > 
> > So you can merge these three patches if you plan to announce a new
> > xfstests version in this weekend.
> > 
> > What do you think about it?
> 
> Sure, you can send V4 of patch 1/5 ~ 3/5 (base on latest for-next branch
> please), as they have been reviewed and tested. Christian's patch (about
> refactor idmapped testing) might need more review, he just sent it out to
> get some review points I think (cc Christian).

LSFMM happened last week so with travel and conference there simply was
no time to rebase. It should be ready for merging once rebased.

> 
> If you'd like to catch up the release of this weekend, please send your
> v4 patch ASAP. Due to I need time to do regression test before pushing.
> It'll wait for next week if too late.

Rebasing the patchset is _massively_ painful which is why in the cover
letter to it I requested that patches which is why I requested
that currently pending patchsets that touch the same code please be
applied on top of it. (I'm happy to apply them manually on top of my
branch.)

In any case, I'll have a rebased version ready on Monday (If there's no
urgent issues I have to address somewhere else that I missed during
travel.)

Christian
Zorro Lang May 7, 2022, 12:26 p.m. UTC | #6
On Sat, May 07, 2022 at 01:40:32PM +0200, Christian Brauner wrote:
> On Sat, May 07, 2022 at 04:52:09PM +0800, Zorro Lang wrote:
> > On Sat, May 07, 2022 at 01:33:33AM +0000, xuyang2018.jy@fujitsu.com wrote:
> > > Hi Zorro
> > > 
> > > Since  Christian doesn't send  a new patchset(for rename idmap-mount)
> > > based on lastest xfstests, should I send a v4 patch for the following
> > > patches today?
> > > "idmapped-mounts: Reset errno to zero after detect fs_allow_idmap"
> > > " idmapped-mounts: Add mknodat operation in setgid test"
> > > "idmapped-mounts: Add open with O_TMPFILE operation in setgid test"
> > > 
> > > So you can merge these three patches if you plan to announce a new
> > > xfstests version in this weekend.
> > > 
> > > What do you think about it?
> > 
> > Sure, you can send V4 of patch 1/5 ~ 3/5 (base on latest for-next branch
> > please), as they have been reviewed and tested. Christian's patch (about
> > refactor idmapped testing) might need more review, he just sent it out to
> > get some review points I think (cc Christian).
> 
> LSFMM happened last week so with travel and conference there simply was
> no time to rebase. It should be ready for merging once rebased.

Yes, there's only a few of patches be reviewed this week, due to most of
related people are in LSF meeting :)

> 
> > 
> > If you'd like to catch up the release of this weekend, please send your
> > v4 patch ASAP. Due to I need time to do regression test before pushing.
> > It'll wait for next week if too late.
> 
> Rebasing the patchset is _massively_ painful which is why in the cover
> letter to it I requested that patches which is why I requested
> that currently pending patchsets that touch the same code please be
> applied on top of it. (I'm happy to apply them manually on top of my
> branch.)

Understand, I did tend to merge your change soon, due to it changes the
fundament of whole idmapped test, but I can't refused other people's
patches if they're ready. One of you or XuYang has to do once rebase at
least, so I have to comply with the sequence of patch reviewed.

If you'd like to merge your patch next week, I'll notice others wait one
more week in the [ANNOUNCE] of this week, if they want to write idmapped
related patches. Is that good to you? Please try to give your patch enough
test, make sure it won't bring in big regression, then we can merge it
quickly and smoothly :)

Thanks,
Zorro

> 
> In any case, I'll have a rebased version ready on Monday (If there's no
> urgent issues I have to address somewhere else that I missed during
> travel.)
> 
> Christian
>
diff mbox series

Patch

diff --git a/src/idmapped-mounts/idmapped-mounts.c b/src/idmapped-mounts/idmapped-mounts.c
index 4cf6c3bb..8e6405c5 100644
--- a/src/idmapped-mounts/idmapped-mounts.c
+++ b/src/idmapped-mounts/idmapped-mounts.c
@@ -14070,6 +14070,8 @@  int main(int argc, char *argv[])
 		die("failed to open %s", t_mountpoint_scratch);
 
 	t_fs_allow_idmap = fs_allow_idmap();
+	/* don't copy ENOSYS errno to child process on older kernel */
+	errno = 0;
 	if (supported) {
 		/*
 		 * Caller just wants to know whether the filesystem we're on