[PATCH iproute2] testsuite: don't clobber /tmp

[PATCH iproute2] testsuite: don't clobber /tmp

[Matteo Croce]

Even if not running the testsuite, every build will leave
a stale tc_testkenv.* file in the system temp directory.
Conditionally create the temp file only if we're running the testsuite.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 testsuite/Makefile | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/testsuite/Makefile b/testsuite/Makefile
index 7f247bbc..5353244b 100644
--- a/testsuite/Makefile
+++ b/testsuite/Makefile
@@ -14,7 +14,9 @@ TESTS_DIR := $(dir $(TESTS))
 
 IPVERS := $(filter-out iproute2/Makefile,$(wildcard iproute2/*))
 
-KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
+ifeq ($(MAKECMDGOALS),alltests)
+	KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
+endif
 ifneq (,$(wildcard /proc/config.gz))
 	KCPATH := /proc/config.gz
 else
@@ -94,3 +96,4 @@ endif
 		rm "$$TMP_ERR" "$$TMP_OUT"; \
 		sudo dmesg > $(RESULTS_DIR)/$@.$$o.dmesg; \
 	done
+	@$(RM) $(KENVFN)
-- 
2.21.0

Re: [PATCH iproute2] testsuite: don't clobber /tmp

[Stephen Hemminger]

On Tue, 11 Jun 2019 20:03:26 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> Even if not running the testsuite, every build will leave
> a stale tc_testkenv.* file in the system temp directory.
> Conditionally create the temp file only if we're running the testsuite.
> 
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---
>  testsuite/Makefile | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/testsuite/Makefile b/testsuite/Makefile
> index 7f247bbc..5353244b 100644
> --- a/testsuite/Makefile
> +++ b/testsuite/Makefile
> @@ -14,7 +14,9 @@ TESTS_DIR := $(dir $(TESTS))
>  
>  IPVERS := $(filter-out iproute2/Makefile,$(wildcard iproute2/*))
>  
> -KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> +ifeq ($(MAKECMDGOALS),alltests)
> +	KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> +endif
>  ifneq (,$(wildcard /proc/config.gz))
>  	KCPATH := /proc/config.gz
>  else
> @@ -94,3 +96,4 @@ endif
>  		rm "$$TMP_ERR" "$$TMP_OUT"; \
>  		sudo dmesg > $(RESULTS_DIR)/$@.$$o.dmesg; \
>  	done
> +	@$(RM) $(KENVFN)

My concern is that there are several targets in this one Makefile.

Why not use -u which gives name but does not create the file?

Re: [PATCH iproute2] testsuite: don't clobber /tmp

[Matteo Croce]

On Wed, Jun 12, 2019 at 5:55 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 11 Jun 2019 20:03:26 +0200
> Matteo Croce <mcroce@redhat.com> wrote:
>
> > Even if not running the testsuite, every build will leave
> > a stale tc_testkenv.* file in the system temp directory.
> > Conditionally create the temp file only if we're running the testsuite.
> >
> > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> > ---
> >  testsuite/Makefile | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/testsuite/Makefile b/testsuite/Makefile
> > index 7f247bbc..5353244b 100644
> > --- a/testsuite/Makefile
> > +++ b/testsuite/Makefile
> > @@ -14,7 +14,9 @@ TESTS_DIR := $(dir $(TESTS))
> >
> >  IPVERS := $(filter-out iproute2/Makefile,$(wildcard iproute2/*))
> >
> > -KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > +ifeq ($(MAKECMDGOALS),alltests)
> > +     KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > +endif
> >  ifneq (,$(wildcard /proc/config.gz))
> >       KCPATH := /proc/config.gz
> >  else
> > @@ -94,3 +96,4 @@ endif
> >               rm "$$TMP_ERR" "$$TMP_OUT"; \
> >               sudo dmesg > $(RESULTS_DIR)/$@.$$o.dmesg; \
> >       done
> > +     @$(RM) $(KENVFN)
>
> My concern is that there are several targets in this one Makefile.
>
> Why not use -u which gives name but does not create the file?

As the manpage says, this is unsafe, as a file with the same name can
be created in the meantime.
Another option is to run the mktemp in the target shell, but this will
require to escape every single end of line to make it a single shell
command, e.g.:

        KENVFN=$$(mktemp /tmp/tc_testkenv.XXXXXX); \
        if [ "$(KCPATH)" = "/proc/config.gz" ]; then \
                gunzip -c $(KCPATH) >$$KENVFN; \
        ...
        done ; \
        $(RM) $$KENVFN

-- 
Matteo Croce
per aspera ad upstream

Re: [PATCH iproute2] testsuite: don't clobber /tmp

[Matteo Croce]

On Wed, Jun 12, 2019 at 6:04 PM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Wed, Jun 12, 2019 at 5:55 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Tue, 11 Jun 2019 20:03:26 +0200
> > Matteo Croce <mcroce@redhat.com> wrote:
> >
> > > Even if not running the testsuite, every build will leave
> > > a stale tc_testkenv.* file in the system temp directory.
> > > Conditionally create the temp file only if we're running the testsuite.
> > >
> > > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> > > ---
> > >  testsuite/Makefile | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/testsuite/Makefile b/testsuite/Makefile
> > > index 7f247bbc..5353244b 100644
> > > --- a/testsuite/Makefile
> > > +++ b/testsuite/Makefile
> > > @@ -14,7 +14,9 @@ TESTS_DIR := $(dir $(TESTS))
> > >
> > >  IPVERS := $(filter-out iproute2/Makefile,$(wildcard iproute2/*))
> > >
> > > -KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > > +ifeq ($(MAKECMDGOALS),alltests)
> > > +     KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > > +endif
> > >  ifneq (,$(wildcard /proc/config.gz))
> > >       KCPATH := /proc/config.gz
> > >  else
> > > @@ -94,3 +96,4 @@ endif
> > >               rm "$$TMP_ERR" "$$TMP_OUT"; \
> > >               sudo dmesg > $(RESULTS_DIR)/$@.$$o.dmesg; \
> > >       done
> > > +     @$(RM) $(KENVFN)
> >
> > My concern is that there are several targets in this one Makefile.
> >
> > Why not use -u which gives name but does not create the file?
>
> As the manpage says, this is unsafe, as a file with the same name can
> be created in the meantime.
> Another option is to run the mktemp in the target shell, but this will
> require to escape every single end of line to make it a single shell
> command, e.g.:
>
>         KENVFN=$$(mktemp /tmp/tc_testkenv.XXXXXX); \
>         if [ "$(KCPATH)" = "/proc/config.gz" ]; then \
>                 gunzip -c $(KCPATH) >$$KENVFN; \
>         ...
>         done ; \
>         $(RM) $$KENVFN
>
> --
> Matteo Croce
> per aspera ad upstream

Anyway, looking for "tc" instead of "alltests" is probably better, as
it only runs mktemp when at least the tc test is selected, both
manually or via make check from topdir, eg.g

ifeq ($(MAKECMDGOALS),tc)

Do you agree?
-- 
Matteo Croce
per aspera ad upstream

Re: [PATCH iproute2] testsuite: don't clobber /tmp

[Stephen Hemminger]

On Wed, 12 Jun 2019 19:32:29 +0200
Matteo Croce <mcroce@redhat.com> wrote:

> On Wed, Jun 12, 2019 at 6:04 PM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > On Wed, Jun 12, 2019 at 5:55 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:  
> > >
> > > On Tue, 11 Jun 2019 20:03:26 +0200
> > > Matteo Croce <mcroce@redhat.com> wrote:
> > >  
> > > > Even if not running the testsuite, every build will leave
> > > > a stale tc_testkenv.* file in the system temp directory.
> > > > Conditionally create the temp file only if we're running the testsuite.
> > > >
> > > > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> > > > ---
> > > >  testsuite/Makefile | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/testsuite/Makefile b/testsuite/Makefile
> > > > index 7f247bbc..5353244b 100644
> > > > --- a/testsuite/Makefile
> > > > +++ b/testsuite/Makefile
> > > > @@ -14,7 +14,9 @@ TESTS_DIR := $(dir $(TESTS))
> > > >
> > > >  IPVERS := $(filter-out iproute2/Makefile,$(wildcard iproute2/*))
> > > >
> > > > -KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > > > +ifeq ($(MAKECMDGOALS),alltests)
> > > > +     KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > > > +endif
> > > >  ifneq (,$(wildcard /proc/config.gz))
> > > >       KCPATH := /proc/config.gz
> > > >  else
> > > > @@ -94,3 +96,4 @@ endif
> > > >               rm "$$TMP_ERR" "$$TMP_OUT"; \
> > > >               sudo dmesg > $(RESULTS_DIR)/$@.$$o.dmesg; \
> > > >       done
> > > > +     @$(RM) $(KENVFN)  
> > >
> > > My concern is that there are several targets in this one Makefile.
> > >
> > > Why not use -u which gives name but does not create the file?  
> >
> > As the manpage says, this is unsafe, as a file with the same name can
> > be created in the meantime.
> > Another option is to run the mktemp in the target shell, but this will
> > require to escape every single end of line to make it a single shell
> > command, e.g.:
> >
> >         KENVFN=$$(mktemp /tmp/tc_testkenv.XXXXXX); \
> >         if [ "$(KCPATH)" = "/proc/config.gz" ]; then \
> >                 gunzip -c $(KCPATH) >$$KENVFN; \
> >         ...
> >         done ; \
> >         $(RM) $$KENVFN
> >
> > --
> > Matteo Croce
> > per aspera ad upstream  
> 
> Anyway, looking for "tc" instead of "alltests" is probably better, as
> it only runs mktemp when at least the tc test is selected, both
> manually or via make check from topdir, eg.g
> 
> ifeq ($(MAKECMDGOALS),tc)
> 
> Do you agree?

Why use /tmp at all for this config file?

Re: [PATCH iproute2] testsuite: don't clobber /tmp

[Matteo Croce]

On Wed, Jun 12, 2019 at 8:20 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Wed, 12 Jun 2019 19:32:29 +0200
> Matteo Croce <mcroce@redhat.com> wrote:
>
> > On Wed, Jun 12, 2019 at 6:04 PM Matteo Croce <mcroce@redhat.com> wrote:
> > >
> > > On Wed, Jun 12, 2019 at 5:55 PM Stephen Hemminger
> > > <stephen@networkplumber.org> wrote:
> > > >
> > > > On Tue, 11 Jun 2019 20:03:26 +0200
> > > > Matteo Croce <mcroce@redhat.com> wrote:
> > > >
> > > > > Even if not running the testsuite, every build will leave
> > > > > a stale tc_testkenv.* file in the system temp directory.
> > > > > Conditionally create the temp file only if we're running the testsuite.
> > > > >
> > > > > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> > > > > ---
> > > > >  testsuite/Makefile | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/testsuite/Makefile b/testsuite/Makefile
> > > > > index 7f247bbc..5353244b 100644
> > > > > --- a/testsuite/Makefile
> > > > > +++ b/testsuite/Makefile
> > > > > @@ -14,7 +14,9 @@ TESTS_DIR := $(dir $(TESTS))
> > > > >
> > > > >  IPVERS := $(filter-out iproute2/Makefile,$(wildcard iproute2/*))
> > > > >
> > > > > -KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > > > > +ifeq ($(MAKECMDGOALS),alltests)
> > > > > +     KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > > > > +endif
> > > > >  ifneq (,$(wildcard /proc/config.gz))
> > > > >       KCPATH := /proc/config.gz
> > > > >  else
> > > > > @@ -94,3 +96,4 @@ endif
> > > > >               rm "$$TMP_ERR" "$$TMP_OUT"; \
> > > > >               sudo dmesg > $(RESULTS_DIR)/$@.$$o.dmesg; \
> > > > >       done
> > > > > +     @$(RM) $(KENVFN)
> > > >
> > > > My concern is that there are several targets in this one Makefile.
> > > >
> > > > Why not use -u which gives name but does not create the file?
> > >
> > > As the manpage says, this is unsafe, as a file with the same name can
> > > be created in the meantime.
> > > Another option is to run the mktemp in the target shell, but this will
> > > require to escape every single end of line to make it a single shell
> > > command, e.g.:
> > >
> > >         KENVFN=$$(mktemp /tmp/tc_testkenv.XXXXXX); \
> > >         if [ "$(KCPATH)" = "/proc/config.gz" ]; then \
> > >                 gunzip -c $(KCPATH) >$$KENVFN; \
> > >         ...
> > >         done ; \
> > >         $(RM) $$KENVFN
> > >
> > > --
> > > Matteo Croce
> > > per aspera ad upstream
> >
> > Anyway, looking for "tc" instead of "alltests" is probably better, as
> > it only runs mktemp when at least the tc test is selected, both
> > manually or via make check from topdir, eg.g
> >
> > ifeq ($(MAKECMDGOALS),tc)
> >
> > Do you agree?
>
> Why use /tmp at all for this config file?

To me any path could work, both /tmp or in the current dir, I have no
preference.
The important thing is to remove them wherever they are, as clobbering
the build dir is bad as messing /tmp.

Anyway, I double checked, and the only target which uses that
temporary file is 'alltests' so, if the path is ok, I think that the
condition "ifeq ($(MAKECMDGOALS),alltests)" is the only one which
fixes the issue and keeps the behaviour unaltered.
I did some quick tests and it works for me.

Bye,
-- 
Matteo Croce
per aspera ad upstream

Re: [PATCH iproute2] testsuite: don't clobber /tmp

[Matteo Croce]

On Thu, Jun 13, 2019 at 7:15 PM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Wed, Jun 12, 2019 at 8:20 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Wed, 12 Jun 2019 19:32:29 +0200
> > Matteo Croce <mcroce@redhat.com> wrote:
> >
> > > On Wed, Jun 12, 2019 at 6:04 PM Matteo Croce <mcroce@redhat.com> wrote:
> > > >
> > > > On Wed, Jun 12, 2019 at 5:55 PM Stephen Hemminger
> > > > <stephen@networkplumber.org> wrote:
> > > > >
> > > > > On Tue, 11 Jun 2019 20:03:26 +0200
> > > > > Matteo Croce <mcroce@redhat.com> wrote:
> > > > >
> > > > > > Even if not running the testsuite, every build will leave
> > > > > > a stale tc_testkenv.* file in the system temp directory.
> > > > > > Conditionally create the temp file only if we're running the testsuite.
> > > > > >
> > > > > > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> > > > > > ---
> > > > > >  testsuite/Makefile | 5 ++++-
> > > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/testsuite/Makefile b/testsuite/Makefile
> > > > > > index 7f247bbc..5353244b 100644
> > > > > > --- a/testsuite/Makefile
> > > > > > +++ b/testsuite/Makefile
> > > > > > @@ -14,7 +14,9 @@ TESTS_DIR := $(dir $(TESTS))
> > > > > >
> > > > > >  IPVERS := $(filter-out iproute2/Makefile,$(wildcard iproute2/*))
> > > > > >
> > > > > > -KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > > > > > +ifeq ($(MAKECMDGOALS),alltests)
> > > > > > +     KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > > > > > +endif
> > > > > >  ifneq (,$(wildcard /proc/config.gz))
> > > > > >       KCPATH := /proc/config.gz
> > > > > >  else
> > > > > > @@ -94,3 +96,4 @@ endif
> > > > > >               rm "$$TMP_ERR" "$$TMP_OUT"; \
> > > > > >               sudo dmesg > $(RESULTS_DIR)/$@.$$o.dmesg; \
> > > > > >       done
> > > > > > +     @$(RM) $(KENVFN)
> > > > >
> > > > > My concern is that there are several targets in this one Makefile.
> > > > >
> > > > > Why not use -u which gives name but does not create the file?
> > > >
> > > > As the manpage says, this is unsafe, as a file with the same name can
> > > > be created in the meantime.
> > > > Another option is to run the mktemp in the target shell, but this will
> > > > require to escape every single end of line to make it a single shell
> > > > command, e.g.:
> > > >
> > > >         KENVFN=$$(mktemp /tmp/tc_testkenv.XXXXXX); \
> > > >         if [ "$(KCPATH)" = "/proc/config.gz" ]; then \
> > > >                 gunzip -c $(KCPATH) >$$KENVFN; \
> > > >         ...
> > > >         done ; \
> > > >         $(RM) $$KENVFN
> > > >
> > > > --
> > > > Matteo Croce
> > > > per aspera ad upstream
> > >
> > > Anyway, looking for "tc" instead of "alltests" is probably better, as
> > > it only runs mktemp when at least the tc test is selected, both
> > > manually or via make check from topdir, eg.g
> > >
> > > ifeq ($(MAKECMDGOALS),tc)
> > >
> > > Do you agree?
> >
> > Why use /tmp at all for this config file?
>
> To me any path could work, both /tmp or in the current dir, I have no
> preference.
> The important thing is to remove them wherever they are, as clobbering
> the build dir is bad as messing /tmp.
>
> Anyway, I double checked, and the only target which uses that
> temporary file is 'alltests' so, if the path is ok, I think that the
> condition "ifeq ($(MAKECMDGOALS),alltests)" is the only one which
> fixes the issue and keeps the behaviour unaltered.
> I did some quick tests and it works for me.
>
> Bye,
> --
> Matteo Croce
> per aspera ad upstream

Hi,

any more thoughts about this patch?

-- 
Matteo Croce
per aspera ad upstream

Re: [PATCH iproute2] testsuite: don't clobber /tmp

[Matteo Croce]

On Tue, Jun 25, 2019 at 4:39 PM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Thu, Jun 13, 2019 at 7:15 PM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > On Wed, Jun 12, 2019 at 8:20 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> >
> > To me any path could work, both /tmp or in the current dir, I have no
> > preference.
> > The important thing is to remove them wherever they are, as clobbering
> > the build dir is bad as messing /tmp.
> >
> > Anyway, I double checked, and the only target which uses that
> > temporary file is 'alltests' so, if the path is ok, I think that the
> > condition "ifeq ($(MAKECMDGOALS),alltests)" is the only one which
> > fixes the issue and keeps the behaviour unaltered.
> > I did some quick tests and it works for me.
> >
> > Bye,
> > --
> > Matteo Croce
> > per aspera ad upstream
>
> Hi,
>
> any more thoughts about this patch?
>
> --
> Matteo Croce
> per aspera ad upstream

Hi,

almost forgot about this one. Should I resend it, or it was nacked?

Regards,
-- 
Matteo Croce
per aspera ad upstream