| Message ID | 166061510477.614432.940552725841920786.stgit@olly (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Ondrej Mosnáček |
| Headers | show |
| Series | [selinux-testsuite] tests/binder: remove stray flag files with 'make clean' | expand |
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.
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?
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.
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.
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
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)
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(-)