| Message ID | 20190611180326.30597-1-mcroce@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2] testsuite: don't clobber /tmp | expand |
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?
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
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?
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?
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,
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?
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,
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)
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(-)