[selinux-testsuite PATCH] tests/binder: remove stray flag files with 'make clean'

[selinux-testsuite PATCH] tests/binder: remove stray flag files with 'make clean'

[Paul Moore]

Failed or incomplete test runs can leave temporary test files in
the binder test directory, remove them with 'make clean'.

  mkfifo: cannot create fifo \
    'binder/manager_flag': File exists
  mkfifo: cannot create fifo \
    'binder/service_provider_flag': File exists

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 tests/binder/Makefile |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/binder/Makefile b/tests/binder/Makefile
index e78ad16..b89d4db 100644
--- a/tests/binder/Makefile
+++ b/tests/binder/Makefile
@@ -18,6 +18,6 @@ endif
 all: $(TARGETS)
 
 clean:
-	rm -f $(TARGETS)
+	rm -f $(TARGETS) manager_flag service_provider_flag
 
 $(TARGETS): $(DEPS)

Re: [selinux-testsuite PATCH] tests/binder: remove stray flag files with 'make clean'

[Ondrej Mosnacek]

On Tue, Aug 16, 2022 at 8:50 AM Paul Moore <paul@paul-moore.com> wrote:
>
> Failed or incomplete test runs can leave temporary test files in
> the binder test directory, remove them with 'make clean'.
>
>   mkfifo: cannot create fifo \
>     'binder/manager_flag': File exists
>   mkfifo: cannot create fifo \
>     'binder/service_provider_flag': File exists
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  tests/binder/Makefile |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/binder/Makefile b/tests/binder/Makefile
> index e78ad16..b89d4db 100644
> --- a/tests/binder/Makefile
> +++ b/tests/binder/Makefile
> @@ -18,6 +18,6 @@ endif
>  all: $(TARGETS)
>
>  clean:
> -       rm -f $(TARGETS)
> +       rm -f $(TARGETS) manager_flag service_provider_flag
>
>  $(TARGETS): $(DEPS)

Thanks, though it would be good to do the same cleanup also in other
tests. I extended your patch to almost all other dirs in [1] - is it
OK if I apply that version?

Note that I didn't bother with the cleanup in filesystem/fs_filesystem
tests, since the tmp dir(s) there may have mounts on them and that's
harder to clean up reliably.

[1] https://github.com/WOnder93/selinux-testsuite/commit/0d5c0af63544a606d42bf9bcb905fc7753994c6d

--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [selinux-testsuite PATCH] tests/binder: remove stray flag files with 'make clean'

[Paul Moore]

On Thu, Aug 25, 2022 at 9:25 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Aug 16, 2022 at 8:50 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > Failed or incomplete test runs can leave temporary test files in
> > the binder test directory, remove them with 'make clean'.
> >
> >   mkfifo: cannot create fifo \
> >     'binder/manager_flag': File exists
> >   mkfifo: cannot create fifo \
> >     'binder/service_provider_flag': File exists
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
> >  tests/binder/Makefile |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/binder/Makefile b/tests/binder/Makefile
> > index e78ad16..b89d4db 100644
> > --- a/tests/binder/Makefile
> > +++ b/tests/binder/Makefile
> > @@ -18,6 +18,6 @@ endif
> >  all: $(TARGETS)
> >
> >  clean:
> > -       rm -f $(TARGETS)
> > +       rm -f $(TARGETS) manager_flag service_provider_flag
> >
> >  $(TARGETS): $(DEPS)
>
> Thanks, though it would be good to do the same cleanup also in other
> tests.

I agree, but I didn't/don't have the time to do that for the other
tests, I saw this one and I fixed it :)

> I extended your patch to almost all other dirs in [1] - is it
> OK if I apply that version?

No, but not because I think those changes are wrong, it's because I
don't agree with the approach.  Let me try to explain ...

It is my personal opinion that with few exceptions, a maintainer
should not alter a patch significantly beyond the normal fuzz that can
sometimes happen when there are merge conflicts.  Of course there are
trivial changes sometimes, e.g. a missing semicolon, whitespace
issues, etc. which are okay to fixup (with a note in the commit!), but
changes of more than a couple of lines, or changes that impact the
logic of the patch, are not something a maintainer should be doing as
a normal practice.  I am not a lawyer, so please don't take this as a
valid interpretation of the laws involved, but I can see legal reasons
for this: if the maintainer changes the patch in a significant way, I
imagine that could potentially muddy the idea of authorship, would the
maintainer now also be considered an author of that patch?  How could
one clearly distinguish between the original author's code and the
mainatiner's changes?  Sure, there is the mailing list, but what if
the mailing list is not available and all you have is the git log?
However, the biggest issue that I see is that of community building.
Having a back-and-forth with a patch contributor can help both welcome
them to the community and teach them what is expected from a patch
submission point of view.  It might be easier and quicker for you just
to edit the existing patch, but it's better for the community to take
the time and ask the original submitter if they could make the change.

Does that make sense?

-- 
paul-moore.com

Re: [selinux-testsuite PATCH] tests/binder: remove stray flag files with 'make clean'

[Ondrej Mosnacek]

On Thu, Aug 25, 2022 at 8:54 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Aug 25, 2022 at 9:25 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Tue, Aug 16, 2022 at 8:50 AM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > Failed or incomplete test runs can leave temporary test files in
> > > the binder test directory, remove them with 'make clean'.
> > >
> > >   mkfifo: cannot create fifo \
> > >     'binder/manager_flag': File exists
> > >   mkfifo: cannot create fifo \
> > >     'binder/service_provider_flag': File exists
> > >
> > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > > ---
> > >  tests/binder/Makefile |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tests/binder/Makefile b/tests/binder/Makefile
> > > index e78ad16..b89d4db 100644
> > > --- a/tests/binder/Makefile
> > > +++ b/tests/binder/Makefile
> > > @@ -18,6 +18,6 @@ endif
> > >  all: $(TARGETS)
> > >
> > >  clean:
> > > -       rm -f $(TARGETS)
> > > +       rm -f $(TARGETS) manager_flag service_provider_flag
> > >
> > >  $(TARGETS): $(DEPS)
> >
> > Thanks, though it would be good to do the same cleanup also in other
> > tests.
>
> I agree, but I didn't/don't have the time to do that for the other
> tests, I saw this one and I fixed it :)
>
> > I extended your patch to almost all other dirs in [1] - is it
> > OK if I apply that version?
>
> No, but not because I think those changes are wrong, it's because I
> don't agree with the approach.  Let me try to explain ...
>
> It is my personal opinion that with few exceptions, a maintainer
> should not alter a patch significantly beyond the normal fuzz that can
> sometimes happen when there are merge conflicts.  Of course there are
> trivial changes sometimes, e.g. a missing semicolon, whitespace
> issues, etc. which are okay to fixup (with a note in the commit!), but
> changes of more than a couple of lines, or changes that impact the
> logic of the patch, are not something a maintainer should be doing as
> a normal practice.  I am not a lawyer, so please don't take this as a
> valid interpretation of the laws involved, but I can see legal reasons
> for this: if the maintainer changes the patch in a significant way, I
> imagine that could potentially muddy the idea of authorship, would the
> maintainer now also be considered an author of that patch?  How could
> one clearly distinguish between the original author's code and the
> mainatiner's changes?  Sure, there is the mailing list, but what if
> the mailing list is not available and all you have is the git log?
> However, the biggest issue that I see is that of community building.
> Having a back-and-forth with a patch contributor can help both welcome
> them to the community and teach them what is expected from a patch
> submission point of view.  It might be easier and quicker for you just
> to edit the existing patch, but it's better for the community to take
> the time and ask the original submitter if they could make the change.
>
> Does that make sense?

Yes, I agree. Looking back at what I did with the patch and
communication I can see I went the completely wrong way about it and I
apologize.

Actually there is really no good reason for me not to simply add the
extra changes as a separate patch/commit... I guess I just
instinctively tried to keep the one logical change in one patch and
didn't think it through properly... I'll scratch that embarrassing
commit and just post the extra changes as another patch.

-- 
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [selinux-testsuite PATCH] tests/binder: remove stray flag files with 'make clean'

[Paul Moore]

On Fri, Aug 26, 2022 at 3:34 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Aug 25, 2022 at 8:54 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Aug 25, 2022 at 9:25 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Tue, Aug 16, 2022 at 8:50 AM Paul Moore <paul@paul-moore.com> wrote:
> > > >
> > > > Failed or incomplete test runs can leave temporary test files in
> > > > the binder test directory, remove them with 'make clean'.
> > > >
> > > >   mkfifo: cannot create fifo \
> > > >     'binder/manager_flag': File exists
> > > >   mkfifo: cannot create fifo \
> > > >     'binder/service_provider_flag': File exists
> > > >
> > > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > > > ---
> > > >  tests/binder/Makefile |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/tests/binder/Makefile b/tests/binder/Makefile
> > > > index e78ad16..b89d4db 100644
> > > > --- a/tests/binder/Makefile
> > > > +++ b/tests/binder/Makefile
> > > > @@ -18,6 +18,6 @@ endif
> > > >  all: $(TARGETS)
> > > >
> > > >  clean:
> > > > -       rm -f $(TARGETS)
> > > > +       rm -f $(TARGETS) manager_flag service_provider_flag
> > > >
> > > >  $(TARGETS): $(DEPS)
> > >
> > > Thanks, though it would be good to do the same cleanup also in other
> > > tests.
> >
> > I agree, but I didn't/don't have the time to do that for the other
> > tests, I saw this one and I fixed it :)
> >
> > > I extended your patch to almost all other dirs in [1] - is it
> > > OK if I apply that version?
> >
> > No, but not because I think those changes are wrong, it's because I
> > don't agree with the approach.  Let me try to explain ...
> >
> > It is my personal opinion that with few exceptions, a maintainer
> > should not alter a patch significantly beyond the normal fuzz that can
> > sometimes happen when there are merge conflicts.  Of course there are
> > trivial changes sometimes, e.g. a missing semicolon, whitespace
> > issues, etc. which are okay to fixup (with a note in the commit!), but
> > changes of more than a couple of lines, or changes that impact the
> > logic of the patch, are not something a maintainer should be doing as
> > a normal practice.  I am not a lawyer, so please don't take this as a
> > valid interpretation of the laws involved, but I can see legal reasons
> > for this: if the maintainer changes the patch in a significant way, I
> > imagine that could potentially muddy the idea of authorship, would the
> > maintainer now also be considered an author of that patch?  How could
> > one clearly distinguish between the original author's code and the
> > mainatiner's changes?  Sure, there is the mailing list, but what if
> > the mailing list is not available and all you have is the git log?
> > However, the biggest issue that I see is that of community building.
> > Having a back-and-forth with a patch contributor can help both welcome
> > them to the community and teach them what is expected from a patch
> > submission point of view.  It might be easier and quicker for you just
> > to edit the existing patch, but it's better for the community to take
> > the time and ask the original submitter if they could make the change.
> >
> > Does that make sense?
>
> Yes, I agree. Looking back at what I did with the patch and
> communication I can see I went the completely wrong way about it and I
> apologize.
>
> Actually there is really no good reason for me not to simply add the
> extra changes as a separate patch/commit... I guess I just
> instinctively tried to keep the one logical change in one patch and
> didn't think it through properly... I'll scratch that embarrassing
> commit and just post the extra changes as another patch.

No worries, thanks for doing the other fixes.

-- 
paul-moore.com

Re: [selinux-testsuite PATCH] tests/binder: remove stray flag files with 'make clean'

[Ondrej Mosnacek]

On Tue, Aug 16, 2022 at 8:50 AM Paul Moore <paul@paul-moore.com> wrote:
> Failed or incomplete test runs can leave temporary test files in
> the binder test directory, remove them with 'make clean'.
>
>   mkfifo: cannot create fifo \
>     'binder/manager_flag': File exists
>   mkfifo: cannot create fifo \
>     'binder/service_provider_flag': File exists
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  tests/binder/Makefile |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/binder/Makefile b/tests/binder/Makefile
> index e78ad16..b89d4db 100644
> --- a/tests/binder/Makefile
> +++ b/tests/binder/Makefile
> @@ -18,6 +18,6 @@ endif
>  all: $(TARGETS)
>
>  clean:
> -       rm -f $(TARGETS)
> +       rm -f $(TARGETS) manager_flag service_provider_flag
>
>  $(TARGETS): $(DEPS)
>

Now applied as:
https://github.com/SELinuxProject/selinux-testsuite/commit/a262824a32b9af008302b4fa42f8c452e3f90c73

-- 
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.