* make notmuch cppcheck clean @ 2017-08-26 14:41 David Bremner 2017-08-26 14:41 ` [PATCH 1/5] build: add target to run cppcheck David Bremner ` (4 more replies) 0 siblings, 5 replies; 11+ messages in thread From: David Bremner @ 2017-08-26 14:41 UTC (permalink / raw) To: notmuch, notmuch I don't know if cppcheck is the best static checker there is, but it's found a few problems already, and I had it installed, so this is an initial response to Jani's suggestion of having one or more targets to run static checkers. I punted on a few of the tricker issues: cppcheck is run serially here, even though it scales rather well, and the target is optional. The rest of the series fixes complaints of cppcheck so that in principle, we could run it automatically. My main reason for not doing so is that I don't want builds that break with a new release of cppcheck. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/5] build: add target to run cppcheck 2017-08-26 14:41 make notmuch cppcheck clean David Bremner @ 2017-08-26 14:41 ` David Bremner 2017-08-27 8:20 ` Jani Nikula 2017-08-26 14:41 ` [PATCH 2/5] cppcheck: close files during shutdown David Bremner ` (3 subsequent siblings) 4 siblings, 1 reply; 11+ messages in thread From: David Bremner @ 2017-08-26 14:41 UTC (permalink / raw) To: notmuch, notmuch The advantage of having a target as opposed to running cppcheck by hand - reuse list of source files - output errors in a format parsable, e.g. by emacs - returns exit code 1 on any error, for possibly use in other targets. Things not addressed here - parallelism. Doing this correctly seems like a rabbit-hole. - what target to invoke this from. The 8s delay (on my machine) seems acceptable, but not necessarily the resulting fragility in autobuilders. --- Makefile.local | 10 ++++++++++ configure | 11 +++++++++++ 2 files changed, 21 insertions(+) diff --git a/Makefile.local b/Makefile.local index af12ca7f..6a5168f1 100644 --- a/Makefile.local +++ b/Makefile.local @@ -283,6 +283,16 @@ CLEAN := $(CLEAN) version.stamp notmuch-*.tar.gz.tmp DISTCLEAN := $(DISTCLEAN) .first-build-message Makefile.config sh.config +CLEAN := $(CLEAN) cppcheck.stamp +cppcheck: cppcheck.stamp +cppcheck.stamp: $(SRCS) +ifeq ($(HAVE_CPPCHECK),1) + cppcheck --template="{file}:{line}: {severity}: {message}" --quiet --error-exitcode=1 ${SRCS} + touch $@ +else + @echo No cppcheck installed; skipping static checking +endif + DEPS := $(SRCS:%.c=.deps/%.d) DEPS := $(DEPS:%.cc=.deps/%.d) -include $(DEPS) diff --git a/configure b/configure index c5e2ffed..364854f3 100755 --- a/configure +++ b/configure @@ -646,6 +646,14 @@ if [ $WITH_DESKTOP = "1" ]; then fi fi +printf "Checking for cppcheck... " +if command -v cppcheck > /dev/null; then + have_cppcheck=1 + printf "Yes.\n" +else + printf "No.\n" +fi + libdir_in_ldconfig=0 printf "Checking which platform we are on... " @@ -1065,6 +1073,9 @@ zsh_completion_dir = ${ZSHCOMLETIONDIR:=\$(prefix)/share/zsh/functions/Completio # build its own version) HAVE_CANONICALIZE_FILE_NAME = ${have_canonicalize_file_name} +# Whether the cppcheck static checker is available +HAVE_CPPCHECK = ${have_cppcheck} + # Whether the getline function is available (if not, then notmuch will # build its own version) HAVE_GETLINE = ${have_getline} -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] build: add target to run cppcheck 2017-08-26 14:41 ` [PATCH 1/5] build: add target to run cppcheck David Bremner @ 2017-08-27 8:20 ` Jani Nikula 2017-08-29 11:35 ` [Patch v2] " David Bremner 0 siblings, 1 reply; 11+ messages in thread From: Jani Nikula @ 2017-08-27 8:20 UTC (permalink / raw) To: David Bremner, notmuch, notmuch On Sat, 26 Aug 2017, David Bremner <david@tethera.net> wrote: > The advantage of having a target as opposed to running cppcheck by > hand > > - reuse list of source files > - output errors in a format parsable, e.g. by emacs > - returns exit code 1 on any error, for possibly use in other > targets. > > Things not addressed here > > - parallelism. Doing this correctly seems like a rabbit-hole. > > - what target to invoke this from. The 8s delay (on my machine) seems > acceptable, but not necessarily the resulting fragility in > autobuilders. Perhaps something could be build around something like below. It addresses the parallelism, but it's not without problems either. Only the files that need to be built would get checked; OTOH that may be beneficial while developing. Clearly we don't want to run cppcheck unconditionally when it's available. Would it be a configure option that would not be enabled by default? Or a make variable passed on the command like, say 'make CPPCHECK=1'? Food for thought; I'm not opposed to applying the patch at hand either. BR, Jani. diff --git a/Makefile.local b/Makefile.local index 6a5168f16427..fd59feafcd44 100644 --- a/Makefile.local +++ b/Makefile.local @@ -191,12 +191,20 @@ endif # Otherwise, print the full command line. quiet ?= $($(shell echo $1 | sed -e s'/ .*//')) +CPPCHECK=cppcheck + %.o: %.cc $(global_deps) @mkdir -p $(patsubst %/.,%,.deps/$(@D)) +ifeq ($(HAVE_CPPCHECK),1) + $(call quiet,CPPCHECK) --template="{file}:{line}: {severity}: {message}" --quiet --error-exitcode=1 $< +endif $(call quiet,CXX $(CPPFLAGS) $(CXXFLAGS)) -c $(FINAL_CXXFLAGS) $< -o $@ -MD -MP -MF .deps/$*.d %.o: %.c $(global_deps) @mkdir -p $(patsubst %/.,%,.deps/$(@D)) +ifeq ($(HAVE_CPPCHECK),1) + $(call quiet,CPPCHECK) --template="{file}:{line}: {severity}: {message}" --quiet --error-exitcode=1 $< +endif $(call quiet,CC $(CPPFLAGS) $(CFLAGS)) -c $(FINAL_CFLAGS) $< -o $@ -MD -MP -MF .deps/$*.d .PHONY : clean > --- > Makefile.local | 10 ++++++++++ > configure | 11 +++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/Makefile.local b/Makefile.local > index af12ca7f..6a5168f1 100644 > --- a/Makefile.local > +++ b/Makefile.local > @@ -283,6 +283,16 @@ CLEAN := $(CLEAN) version.stamp notmuch-*.tar.gz.tmp > > DISTCLEAN := $(DISTCLEAN) .first-build-message Makefile.config sh.config > > +CLEAN := $(CLEAN) cppcheck.stamp > +cppcheck: cppcheck.stamp > +cppcheck.stamp: $(SRCS) > +ifeq ($(HAVE_CPPCHECK),1) > + cppcheck --template="{file}:{line}: {severity}: {message}" --quiet --error-exitcode=1 ${SRCS} > + touch $@ > +else > + @echo No cppcheck installed; skipping static checking > +endif > + > DEPS := $(SRCS:%.c=.deps/%.d) > DEPS := $(DEPS:%.cc=.deps/%.d) > -include $(DEPS) > diff --git a/configure b/configure > index c5e2ffed..364854f3 100755 > --- a/configure > +++ b/configure > @@ -646,6 +646,14 @@ if [ $WITH_DESKTOP = "1" ]; then > fi > fi > > +printf "Checking for cppcheck... " > +if command -v cppcheck > /dev/null; then > + have_cppcheck=1 > + printf "Yes.\n" > +else > + printf "No.\n" > +fi > + > libdir_in_ldconfig=0 > > printf "Checking which platform we are on... " > @@ -1065,6 +1073,9 @@ zsh_completion_dir = ${ZSHCOMLETIONDIR:=\$(prefix)/share/zsh/functions/Completio > # build its own version) > HAVE_CANONICALIZE_FILE_NAME = ${have_canonicalize_file_name} > > +# Whether the cppcheck static checker is available > +HAVE_CPPCHECK = ${have_cppcheck} > + > # Whether the getline function is available (if not, then notmuch will > # build its own version) > HAVE_GETLINE = ${have_getline} > -- > 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Patch v2] build: add target to run cppcheck 2017-08-27 8:20 ` Jani Nikula @ 2017-08-29 11:35 ` David Bremner 2017-08-29 18:36 ` Jani Nikula 0 siblings, 1 reply; 11+ messages in thread From: David Bremner @ 2017-08-29 11:35 UTC (permalink / raw) To: Jani Nikula, David Bremner, notmuch, notmuch The advantage of having a target as opposed to running cppcheck by hand - reuse list of source files - output errors in a format parsable, e.g. by emacs - returns exit code 1 on any error, for possibly use in other targets. For the moment, leave this as an optional target. If desired, it can be added to e.g. the release targets in the same way as the test target. Using two levels of directory for the stamps is arguably overengineering, but it doesn't really cost anything, and leaves open the possibility of putting other kinds of stamp files there. This only checks "new" source files (w.r.t. their last check). A future target (cppcheck-all ?) could blow away the stamp files first. --- Two notable changes from the first version: 1) support parallel build, inspired by id:87pobhl8wc.fsf@nikula.org; 2) use the predefined 'gcc' template. It's still an optional target, as that felt most comfortable as a workflow to me. Makefile.local | 18 ++++++++++++++++++ configure | 11 +++++++++++ 2 files changed, 29 insertions(+) diff --git a/Makefile.local b/Makefile.local index af12ca7f..c6ad0047 100644 --- a/Makefile.local +++ b/Makefile.local @@ -199,6 +199,14 @@ quiet ?= $($(shell echo $1 | sed -e s'/ .*//')) @mkdir -p $(patsubst %/.,%,.deps/$(@D)) $(call quiet,CC $(CPPFLAGS) $(CFLAGS)) -c $(FINAL_CFLAGS) $< -o $@ -MD -MP -MF .deps/$*.d +CPPCHECK=cppcheck +.stamps/cppcheck/%: % + @mkdir -p $(@D) + $(call quiet,CPPCHECK) --template=gcc --error-exitcode=1 --quiet $< + @touch $@ + +CLEAN := $(CLEAN) .stamps + .PHONY : clean clean: rm -rf $(CLEAN); rm -rf .deps @@ -283,6 +291,16 @@ CLEAN := $(CLEAN) version.stamp notmuch-*.tar.gz.tmp DISTCLEAN := $(DISTCLEAN) .first-build-message Makefile.config sh.config +CPPCHECK_STAMPS := $(SRCS:%=.stamps/cppcheck/%) +.PHONY: cppcheck +ifeq ($(HAVE_CPPCHECK),1) +cppcheck: ${CPPCHECK_STAMPS} +else +cppcheck: + @echo "No cppcheck found during configure; skipping static checking" +endif + + DEPS := $(SRCS:%.c=.deps/%.d) DEPS := $(DEPS:%.cc=.deps/%.d) -include $(DEPS) diff --git a/configure b/configure index c5e2ffed..364854f3 100755 --- a/configure +++ b/configure @@ -646,6 +646,14 @@ if [ $WITH_DESKTOP = "1" ]; then fi fi +printf "Checking for cppcheck... " +if command -v cppcheck > /dev/null; then + have_cppcheck=1 + printf "Yes.\n" +else + printf "No.\n" +fi + libdir_in_ldconfig=0 printf "Checking which platform we are on... " @@ -1065,6 +1073,9 @@ zsh_completion_dir = ${ZSHCOMLETIONDIR:=\$(prefix)/share/zsh/functions/Completio # build its own version) HAVE_CANONICALIZE_FILE_NAME = ${have_canonicalize_file_name} +# Whether the cppcheck static checker is available +HAVE_CPPCHECK = ${have_cppcheck} + # Whether the getline function is available (if not, then notmuch will # build its own version) HAVE_GETLINE = ${have_getline} -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Patch v2] build: add target to run cppcheck 2017-08-29 11:35 ` [Patch v2] " David Bremner @ 2017-08-29 18:36 ` Jani Nikula 2017-08-29 18:47 ` Jani Nikula 2017-08-30 23:28 ` David Bremner 0 siblings, 2 replies; 11+ messages in thread From: Jani Nikula @ 2017-08-29 18:36 UTC (permalink / raw) To: David Bremner, David Bremner, notmuch, notmuch On Tue, 29 Aug 2017, David Bremner <david@tethera.net> wrote: > The advantage of having a target as opposed to running cppcheck by > hand > > - reuse list of source files > - output errors in a format parsable, e.g. by emacs > - returns exit code 1 on any error, for possibly use in other > targets. > > For the moment, leave this as an optional target. If desired, it can > be added to e.g. the release targets in the same way as the test > target. > > Using two levels of directory for the stamps is arguably > overengineering, but it doesn't really cost anything, and leaves open > the possibility of putting other kinds of stamp files there. > > This only checks "new" source files (w.r.t. their last check). A future target > (cppcheck-all ?) could blow away the stamp files first. > > --- > > Two notable changes from the first version: 1) support parallel build, > inspired by id:87pobhl8wc.fsf@nikula.org; 2) use the predefined 'gcc' > template. It's still an optional target, as that felt most comfortable > as a workflow to me. This is nice. LGTM. I might have left out --error-exitcode=1 myself but no strong feelings. I thought the quiet build lines like CPPCHECK .stamps/cppcheck/util/error_util.c were a bit ugly, so I came up with [1]. With that plus this change: - $(call quiet,CPPCHECK) --template=gcc --error-exitcode=1 --quiet $< + $(call quiet,CPPCHECK,$<) --template=gcc --error-exitcode=1 --quiet $< you get CPPCHECK util/error_util.c BR, Jani. [1] id:20170829182709.26841-2-jani@nikula.org > > Makefile.local | 18 ++++++++++++++++++ > configure | 11 +++++++++++ > 2 files changed, 29 insertions(+) > > diff --git a/Makefile.local b/Makefile.local > index af12ca7f..c6ad0047 100644 > --- a/Makefile.local > +++ b/Makefile.local > @@ -199,6 +199,14 @@ quiet ?= $($(shell echo $1 | sed -e s'/ .*//')) > @mkdir -p $(patsubst %/.,%,.deps/$(@D)) > $(call quiet,CC $(CPPFLAGS) $(CFLAGS)) -c $(FINAL_CFLAGS) $< -o $@ -MD -MP -MF .deps/$*.d > > +CPPCHECK=cppcheck > +.stamps/cppcheck/%: % > + @mkdir -p $(@D) > + $(call quiet,CPPCHECK) --template=gcc --error-exitcode=1 --quiet $< > + @touch $@ > + > +CLEAN := $(CLEAN) .stamps > + > .PHONY : clean > clean: > rm -rf $(CLEAN); rm -rf .deps > @@ -283,6 +291,16 @@ CLEAN := $(CLEAN) version.stamp notmuch-*.tar.gz.tmp > > DISTCLEAN := $(DISTCLEAN) .first-build-message Makefile.config sh.config > > +CPPCHECK_STAMPS := $(SRCS:%=.stamps/cppcheck/%) > +.PHONY: cppcheck > +ifeq ($(HAVE_CPPCHECK),1) > +cppcheck: ${CPPCHECK_STAMPS} > +else > +cppcheck: > + @echo "No cppcheck found during configure; skipping static checking" > +endif > + > + > DEPS := $(SRCS:%.c=.deps/%.d) > DEPS := $(DEPS:%.cc=.deps/%.d) > -include $(DEPS) > diff --git a/configure b/configure > index c5e2ffed..364854f3 100755 > --- a/configure > +++ b/configure > @@ -646,6 +646,14 @@ if [ $WITH_DESKTOP = "1" ]; then > fi > fi > > +printf "Checking for cppcheck... " > +if command -v cppcheck > /dev/null; then > + have_cppcheck=1 > + printf "Yes.\n" > +else > + printf "No.\n" > +fi > + > libdir_in_ldconfig=0 > > printf "Checking which platform we are on... " > @@ -1065,6 +1073,9 @@ zsh_completion_dir = ${ZSHCOMLETIONDIR:=\$(prefix)/share/zsh/functions/Completio > # build its own version) > HAVE_CANONICALIZE_FILE_NAME = ${have_canonicalize_file_name} > > +# Whether the cppcheck static checker is available > +HAVE_CPPCHECK = ${have_cppcheck} > + > # Whether the getline function is available (if not, then notmuch will > # build its own version) > HAVE_GETLINE = ${have_getline} > -- > 2.14.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch v2] build: add target to run cppcheck 2017-08-29 18:36 ` Jani Nikula @ 2017-08-29 18:47 ` Jani Nikula 2017-08-30 23:28 ` David Bremner 1 sibling, 0 replies; 11+ messages in thread From: Jani Nikula @ 2017-08-29 18:47 UTC (permalink / raw) To: David Bremner, David Bremner, notmuch, notmuch On Tue, 29 Aug 2017, Jani Nikula <jani@nikula.org> wrote: > On Tue, 29 Aug 2017, David Bremner <david@tethera.net> wrote: >> The advantage of having a target as opposed to running cppcheck by >> hand >> >> - reuse list of source files >> - output errors in a format parsable, e.g. by emacs >> - returns exit code 1 on any error, for possibly use in other >> targets. >> >> For the moment, leave this as an optional target. If desired, it can >> be added to e.g. the release targets in the same way as the test >> target. >> >> Using two levels of directory for the stamps is arguably >> overengineering, but it doesn't really cost anything, and leaves open >> the possibility of putting other kinds of stamp files there. >> >> This only checks "new" source files (w.r.t. their last check). A future target >> (cppcheck-all ?) could blow away the stamp files first. >> >> --- >> >> Two notable changes from the first version: 1) support parallel build, >> inspired by id:87pobhl8wc.fsf@nikula.org; 2) use the predefined 'gcc' >> template. It's still an optional target, as that felt most comfortable >> as a workflow to me. > > This is nice. LGTM. I might have left out --error-exitcode=1 myself but > no strong feelings. Ah, I realize without --error-exitcode=1 you won't see the errors anymore. Nevermind. The fixes LGTM good. BR, Jani. > > I thought the quiet build lines like > > CPPCHECK .stamps/cppcheck/util/error_util.c > > were a bit ugly, so I came up with [1]. With that plus this change: > > - $(call quiet,CPPCHECK) --template=gcc --error-exitcode=1 --quiet $< > + $(call quiet,CPPCHECK,$<) --template=gcc --error-exitcode=1 --quiet $< > > you get > > CPPCHECK util/error_util.c > > > BR, > Jani. > > > [1] id:20170829182709.26841-2-jani@nikula.org > >> >> Makefile.local | 18 ++++++++++++++++++ >> configure | 11 +++++++++++ >> 2 files changed, 29 insertions(+) >> >> diff --git a/Makefile.local b/Makefile.local >> index af12ca7f..c6ad0047 100644 >> --- a/Makefile.local >> +++ b/Makefile.local >> @@ -199,6 +199,14 @@ quiet ?= $($(shell echo $1 | sed -e s'/ .*//')) >> @mkdir -p $(patsubst %/.,%,.deps/$(@D)) >> $(call quiet,CC $(CPPFLAGS) $(CFLAGS)) -c $(FINAL_CFLAGS) $< -o $@ -MD -MP -MF .deps/$*.d >> >> +CPPCHECK=cppcheck >> +.stamps/cppcheck/%: % >> + @mkdir -p $(@D) >> + $(call quiet,CPPCHECK) --template=gcc --error-exitcode=1 --quiet $< >> + @touch $@ >> + >> +CLEAN := $(CLEAN) .stamps >> + >> .PHONY : clean >> clean: >> rm -rf $(CLEAN); rm -rf .deps >> @@ -283,6 +291,16 @@ CLEAN := $(CLEAN) version.stamp notmuch-*.tar.gz.tmp >> >> DISTCLEAN := $(DISTCLEAN) .first-build-message Makefile.config sh.config >> >> +CPPCHECK_STAMPS := $(SRCS:%=.stamps/cppcheck/%) >> +.PHONY: cppcheck >> +ifeq ($(HAVE_CPPCHECK),1) >> +cppcheck: ${CPPCHECK_STAMPS} >> +else >> +cppcheck: >> + @echo "No cppcheck found during configure; skipping static checking" >> +endif >> + >> + >> DEPS := $(SRCS:%.c=.deps/%.d) >> DEPS := $(DEPS:%.cc=.deps/%.d) >> -include $(DEPS) >> diff --git a/configure b/configure >> index c5e2ffed..364854f3 100755 >> --- a/configure >> +++ b/configure >> @@ -646,6 +646,14 @@ if [ $WITH_DESKTOP = "1" ]; then >> fi >> fi >> >> +printf "Checking for cppcheck... " >> +if command -v cppcheck > /dev/null; then >> + have_cppcheck=1 >> + printf "Yes.\n" >> +else >> + printf "No.\n" >> +fi >> + >> libdir_in_ldconfig=0 >> >> printf "Checking which platform we are on... " >> @@ -1065,6 +1073,9 @@ zsh_completion_dir = ${ZSHCOMLETIONDIR:=\$(prefix)/share/zsh/functions/Completio >> # build its own version) >> HAVE_CANONICALIZE_FILE_NAME = ${have_canonicalize_file_name} >> >> +# Whether the cppcheck static checker is available >> +HAVE_CPPCHECK = ${have_cppcheck} >> + >> # Whether the getline function is available (if not, then notmuch will >> # build its own version) >> HAVE_GETLINE = ${have_getline} >> -- >> 2.14.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch v2] build: add target to run cppcheck 2017-08-29 18:36 ` Jani Nikula 2017-08-29 18:47 ` Jani Nikula @ 2017-08-30 23:28 ` David Bremner 1 sibling, 0 replies; 11+ messages in thread From: David Bremner @ 2017-08-30 23:28 UTC (permalink / raw) To: Jani Nikula, notmuch, notmuch Jani Nikula <jani@nikula.org> writes: > > I thought the quiet build lines like > > CPPCHECK .stamps/cppcheck/util/error_util.c > > were a bit ugly, so I came up with [1]. With that plus this change: > > - $(call quiet,CPPCHECK) --template=gcc --error-exitcode=1 --quiet $< > + $(call quiet,CPPCHECK,$<) --template=gcc --error-exitcode=1 --quiet $< > series pushed, with this change d ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/5] cppcheck: close files during shutdown 2017-08-26 14:41 make notmuch cppcheck clean David Bremner 2017-08-26 14:41 ` [PATCH 1/5] build: add target to run cppcheck David Bremner @ 2017-08-26 14:41 ` David Bremner 2017-08-26 14:41 ` [PATCH 3/5] cppcheck: call va_end in _internal_error David Bremner ` (2 subsequent siblings) 4 siblings, 0 replies; 11+ messages in thread From: David Bremner @ 2017-08-26 14:41 UTC (permalink / raw) To: notmuch, notmuch Fix the following cppcheck errors: notmuch-count.c:207: error: Resource leak: input notmuch-tag.c:238: error: Resource leak: input We know that the program is shutting down here, but it does no harm to clean up a bit. --- notmuch-count.c | 2 ++ notmuch-tag.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/notmuch-count.c b/notmuch-count.c index 50b0c193..97281374 100644 --- a/notmuch-count.c +++ b/notmuch-count.c @@ -204,6 +204,8 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[]) if (batch && opt_index != argc) { fprintf (stderr, "--batch and query string are not compatible\n"); + if (input) + fclose (input); return EXIT_FAILURE; } diff --git a/notmuch-tag.c b/notmuch-tag.c index 9c03754d..130de634 100644 --- a/notmuch-tag.c +++ b/notmuch-tag.c @@ -235,6 +235,8 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[]) if (batch) { if (opt_index != argc) { fprintf (stderr, "Can't specify both cmdline and stdin!\n"); + if (input) + fclose (input); return EXIT_FAILURE; } } else { -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/5] cppcheck: call va_end in _internal_error 2017-08-26 14:41 make notmuch cppcheck clean David Bremner 2017-08-26 14:41 ` [PATCH 1/5] build: add target to run cppcheck David Bremner 2017-08-26 14:41 ` [PATCH 2/5] cppcheck: close files during shutdown David Bremner @ 2017-08-26 14:41 ` David Bremner 2017-08-26 14:41 ` [PATCH 4/5] test/smtp-dummy: uncrustify David Bremner 2017-08-26 14:41 ` [PATCH 5/5] test/smtp-dummy: convert to 'goto DONE' style David Bremner 4 siblings, 0 replies; 11+ messages in thread From: David Bremner @ 2017-08-26 14:41 UTC (permalink / raw) To: notmuch, notmuch fix for: util/error_util.c:38: error: va_list 'va_args' was opened but not closed by va_end() This makes the code more copy-pastable, if nothing else --- util/error_util.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/error_util.c b/util/error_util.c index 778bbd52..e64162c7 100644 --- a/util/error_util.c +++ b/util/error_util.c @@ -34,6 +34,7 @@ _internal_error (const char *format, ...) fprintf (stderr, "Internal error: "); vfprintf (stderr, format, va_args); + va_end (va_args); exit (1); } -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/5] test/smtp-dummy: uncrustify 2017-08-26 14:41 make notmuch cppcheck clean David Bremner ` (2 preceding siblings ...) 2017-08-26 14:41 ` [PATCH 3/5] cppcheck: call va_end in _internal_error David Bremner @ 2017-08-26 14:41 ` David Bremner 2017-08-26 14:41 ` [PATCH 5/5] test/smtp-dummy: convert to 'goto DONE' style David Bremner 4 siblings, 0 replies; 11+ messages in thread From: David Bremner @ 2017-08-26 14:41 UTC (permalink / raw) To: notmuch, notmuch For some reason lost in the mists of time this code was indented 8 spaces. --- test/smtp-dummy.c | 336 +++++++++++++++++++++++++++--------------------------- 1 file changed, 168 insertions(+), 168 deletions(-) diff --git a/test/smtp-dummy.c b/test/smtp-dummy.c index a629927c..fec2b8ad 100644 --- a/test/smtp-dummy.c +++ b/test/smtp-dummy.c @@ -49,211 +49,211 @@ static void receive_data_to_file (FILE *peer, FILE *output) { - char *line = NULL; - size_t line_size; - ssize_t line_len; + char *line = NULL; + size_t line_size; + ssize_t line_len; - while ((line_len = getline (&line, &line_size, peer)) != -1) { - if (STRNCMP_LITERAL (line, ".\r\n") == 0) - break; - if (line_len < 2) - continue; - if (line[line_len-1] == '\n' && line[line_len-2] == '\r') { - line[line_len-2] = '\n'; - line[line_len-1] = '\0'; - } - fprintf (output, "%s", - line[0] == '.' ? line + 1 : line); + while ((line_len = getline (&line, &line_size, peer)) != -1) { + if (STRNCMP_LITERAL (line, ".\r\n") == 0) + break; + if (line_len < 2) + continue; + if (line[line_len - 1] == '\n' && line[line_len - 2] == '\r') { + line[line_len - 2] = '\n'; + line[line_len - 1] = '\0'; } + fprintf (output, "%s", + line[0] == '.' ? line + 1 : line); + } - free (line); + free (line); } static int process_command (FILE *peer, FILE *output, const char *command) { - if (STRNCMP_LITERAL (command, "EHLO ") == 0) { - fprintf (peer, "502 not implemented\r\n"); - fflush (peer); - } else if (STRNCMP_LITERAL (command, "HELO ") == 0) { - fprintf (peer, "250 localhost\r\n"); - fflush (peer); - } else if (STRNCMP_LITERAL (command, "MAIL FROM:") == 0 || - STRNCMP_LITERAL (command, "RCPT TO:") == 0) { - fprintf (peer, "250 OK\r\n"); - fflush (peer); - } else if (STRNCMP_LITERAL (command, "DATA") == 0) { - fprintf (peer, "354 End data with <CR><LF>.<CR><LF>\r\n"); - fflush (peer); - receive_data_to_file (peer, output); - fprintf (peer, "250 OK\r\n"); - fflush (peer); - } else if (STRNCMP_LITERAL (command, "QUIT") == 0) { - fprintf (peer, "221 BYE\r\n"); - fflush (peer); - return 1; - } else { - fprintf (stderr, "Unknown command: %s\n", command); - } - return 0; + if (STRNCMP_LITERAL (command, "EHLO ") == 0) { + fprintf (peer, "502 not implemented\r\n"); + fflush (peer); + } else if (STRNCMP_LITERAL (command, "HELO ") == 0) { + fprintf (peer, "250 localhost\r\n"); + fflush (peer); + } else if (STRNCMP_LITERAL (command, "MAIL FROM:") == 0 || + STRNCMP_LITERAL (command, "RCPT TO:") == 0) { + fprintf (peer, "250 OK\r\n"); + fflush (peer); + } else if (STRNCMP_LITERAL (command, "DATA") == 0) { + fprintf (peer, "354 End data with <CR><LF>.<CR><LF>\r\n"); + fflush (peer); + receive_data_to_file (peer, output); + fprintf (peer, "250 OK\r\n"); + fflush (peer); + } else if (STRNCMP_LITERAL (command, "QUIT") == 0) { + fprintf (peer, "221 BYE\r\n"); + fflush (peer); + return 1; + } else { + fprintf (stderr, "Unknown command: %s\n", command); + } + return 0; } static void do_smtp_to_file (FILE *peer, FILE *output) { - char *line = NULL; - size_t line_size; - ssize_t line_len; + char *line = NULL; + size_t line_size; + ssize_t line_len; - fprintf (peer, "220 localhost smtp-dummy\r\n"); - fflush (peer); + fprintf (peer, "220 localhost smtp-dummy\r\n"); + fflush (peer); - while ((line_len = getline (&line, &line_size, peer)) != -1) { - if (process_command (peer, output, line)) - break; - } + while ((line_len = getline (&line, &line_size, peer)) != -1) { + if (process_command (peer, output, line)) + break; + } - free (line); + free (line); } int main (int argc, char *argv[]) { - const char * progname; - char *output_filename; - FILE *peer_file, *output; - int sock, peer, err; - struct sockaddr_in addr, peer_addr; - struct hostent *hostinfo; - socklen_t peer_addr_len; - int reuse; - int background; + const char *progname; + char *output_filename; + FILE *peer_file, *output; + int sock, peer, err; + struct sockaddr_in addr, peer_addr; + struct hostent *hostinfo; + socklen_t peer_addr_len; + int reuse; + int background; - progname = argv[0]; + progname = argv[0]; - background = 0; - for (; argc >= 2; argc--, argv++) { - if (argv[1][0] != '-') - break; - if (strcmp (argv[1], "--") == 0) { - argc--; - argv++; - break; - } - if (strcmp (argv[1], "--background") == 0) { - background = 1; - continue; - } - fprintf(stderr, "%s: unregognized option '%s'\n", - progname, argv[1]); - return 1; + background = 0; + for (; argc >= 2; argc--, argv++) { + if (argv[1][0] != '-') + break; + if (strcmp (argv[1], "--") == 0) { + argc--; + argv++; + break; } - - if (argc != 2) { - fprintf (stderr, - "Usage: %s [--background] <output-file>\n", progname); - return 1; + if (strcmp (argv[1], "--background") == 0) { + background = 1; + continue; } + fprintf (stderr, "%s: unregognized option '%s'\n", + progname, argv[1]); + return 1; + } - output_filename = argv[1]; - output = fopen (output_filename, "w"); - if (output == NULL) { - fprintf (stderr, "Failed to open %s for writing: %s\n", - output_filename, strerror (errno)); - return 1; - } + if (argc != 2) { + fprintf (stderr, + "Usage: %s [--background] <output-file>\n", progname); + return 1; + } - sock = socket (AF_INET, SOCK_STREAM, 0); - if (sock == -1) { - fprintf (stderr, "Error: socket() failed: %s\n", - strerror (errno)); - return 1; - } + output_filename = argv[1]; + output = fopen (output_filename, "w"); + if (output == NULL) { + fprintf (stderr, "Failed to open %s for writing: %s\n", + output_filename, strerror (errno)); + return 1; + } - reuse = 1; - err = setsockopt (sock, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof (reuse)); - if (err) { - fprintf (stderr, "Error: setsockopt() failed: %s\n", - strerror (errno)); - return 1; - } + sock = socket (AF_INET, SOCK_STREAM, 0); + if (sock == -1) { + fprintf (stderr, "Error: socket() failed: %s\n", + strerror (errno)); + return 1; + } - hostinfo = gethostbyname ("localhost"); - if (hostinfo == NULL) { - fprintf (stderr, "Unknown host: localhost\n"); - return 1; - } + reuse = 1; + err = setsockopt (sock, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof (reuse)); + if (err) { + fprintf (stderr, "Error: setsockopt() failed: %s\n", + strerror (errno)); + return 1; + } - memset (&addr, 0, sizeof (addr)); - addr.sin_family = AF_INET; - addr.sin_port = htons (25025); - addr.sin_addr = *(struct in_addr *) hostinfo->h_addr; - err = bind (sock, (struct sockaddr *) &addr, sizeof(addr)); - if (err) { - fprintf (stderr, "Error: bind() failed: %s\n", - strerror (errno)); - close (sock); - return 1; - } + hostinfo = gethostbyname ("localhost"); + if (hostinfo == NULL) { + fprintf (stderr, "Unknown host: localhost\n"); + return 1; + } - err = listen (sock, 1); - if (err) { - fprintf (stderr, "Error: listen() failed: %s\n", - strerror (errno)); - close (sock); - return 1; - } + memset (&addr, 0, sizeof (addr)); + addr.sin_family = AF_INET; + addr.sin_port = htons (25025); + addr.sin_addr = *(struct in_addr *) hostinfo->h_addr; + err = bind (sock, (struct sockaddr *) &addr, sizeof (addr)); + if (err) { + fprintf (stderr, "Error: bind() failed: %s\n", + strerror (errno)); + close (sock); + return 1; + } - if (background) { - int pid = fork (); - if (pid > 0) { - printf ("smtp_dummy_pid='%d'\n", pid); - fflush (stdout); - close (sock); - return 0; - } - if (pid < 0) { - fprintf (stderr, "Error: fork() failed: %s\n", - strerror (errno)); - close (sock); - return 1; - } - /* Reached if pid == 0 (the child process). */ - /* Close stdout so that the one interested in pid value will - also get EOF. */ - close (STDOUT_FILENO); - /* dup2() will re-reserve fd of stdout (1) (opportunistically), - in case fd of stderr (2) is open. If that was not open we - don't care fd of stdout (1) either. */ - dup2 (STDERR_FILENO, STDOUT_FILENO); + err = listen (sock, 1); + if (err) { + fprintf (stderr, "Error: listen() failed: %s\n", + strerror (errno)); + close (sock); + return 1; + } - /* This process is now out of reach of shell's job control. - To resolve the rare but possible condition where this - "daemon" is started but never connected this process will - (only) have 30 seconds to exist. */ - alarm (30); + if (background) { + int pid = fork (); + if (pid > 0) { + printf ("smtp_dummy_pid='%d'\n", pid); + fflush (stdout); + close (sock); + return 0; } - - peer_addr_len = sizeof (peer_addr); - peer = accept (sock, (struct sockaddr *) &peer_addr, &peer_addr_len); - if (peer == -1) { - fprintf (stderr, "Error: accept() failed: %s\n", - strerror (errno)); - return 1; + if (pid < 0) { + fprintf (stderr, "Error: fork() failed: %s\n", + strerror (errno)); + close (sock); + return 1; } + /* Reached if pid == 0 (the child process). */ + /* Close stdout so that the one interested in pid value will + * also get EOF. */ + close (STDOUT_FILENO); + /* dup2() will re-reserve fd of stdout (1) (opportunistically), + * in case fd of stderr (2) is open. If that was not open we + * don't care fd of stdout (1) either. */ + dup2 (STDERR_FILENO, STDOUT_FILENO); - peer_file = fdopen (peer, "w+"); - if (peer_file == NULL) { - fprintf (stderr, "Error: fdopen() failed: %s\n", - strerror (errno)); - return 1; - } + /* This process is now out of reach of shell's job control. + * To resolve the rare but possible condition where this + * "daemon" is started but never connected this process will + * (only) have 30 seconds to exist. */ + alarm (30); + } - do_smtp_to_file (peer_file, output); + peer_addr_len = sizeof (peer_addr); + peer = accept (sock, (struct sockaddr *) &peer_addr, &peer_addr_len); + if (peer == -1) { + fprintf (stderr, "Error: accept() failed: %s\n", + strerror (errno)); + return 1; + } - fclose (output); - fclose (peer_file); - close (sock); + peer_file = fdopen (peer, "w+"); + if (peer_file == NULL) { + fprintf (stderr, "Error: fdopen() failed: %s\n", + strerror (errno)); + return 1; + } + + do_smtp_to_file (peer_file, output); + + fclose (output); + fclose (peer_file); + close (sock); - return 0; + return 0; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 5/5] test/smtp-dummy: convert to 'goto DONE' style 2017-08-26 14:41 make notmuch cppcheck clean David Bremner ` (3 preceding siblings ...) 2017-08-26 14:41 ` [PATCH 4/5] test/smtp-dummy: uncrustify David Bremner @ 2017-08-26 14:41 ` David Bremner 4 siblings, 0 replies; 11+ messages in thread From: David Bremner @ 2017-08-26 14:41 UTC (permalink / raw) To: notmuch, notmuch Clean up several cppcheck warnings of the form - test/smtp-dummy.c:170: error: Resource leak: output Conform to overall notmuch code style. --- test/smtp-dummy.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/test/smtp-dummy.c b/test/smtp-dummy.c index fec2b8ad..71992edd 100644 --- a/test/smtp-dummy.c +++ b/test/smtp-dummy.c @@ -121,13 +121,14 @@ main (int argc, char *argv[]) { const char *progname; char *output_filename; - FILE *peer_file, *output; - int sock, peer, err; + FILE *peer_file = NULL, *output = NULL; + int sock = -1, peer, err; struct sockaddr_in addr, peer_addr; struct hostent *hostinfo; socklen_t peer_addr_len; int reuse; int background; + int ret = 0; progname = argv[0]; @@ -160,14 +161,16 @@ main (int argc, char *argv[]) if (output == NULL) { fprintf (stderr, "Failed to open %s for writing: %s\n", output_filename, strerror (errno)); - return 1; + ret = 1; + goto DONE; } sock = socket (AF_INET, SOCK_STREAM, 0); if (sock == -1) { fprintf (stderr, "Error: socket() failed: %s\n", strerror (errno)); - return 1; + ret = 1; + goto DONE; } reuse = 1; @@ -175,13 +178,15 @@ main (int argc, char *argv[]) if (err) { fprintf (stderr, "Error: setsockopt() failed: %s\n", strerror (errno)); - return 1; + ret = 1; + goto DONE; } hostinfo = gethostbyname ("localhost"); if (hostinfo == NULL) { fprintf (stderr, "Unknown host: localhost\n"); - return 1; + ret = 1; + goto DONE; } memset (&addr, 0, sizeof (addr)); @@ -193,7 +198,8 @@ main (int argc, char *argv[]) fprintf (stderr, "Error: bind() failed: %s\n", strerror (errno)); close (sock); - return 1; + ret = 1; + goto DONE; } err = listen (sock, 1); @@ -201,7 +207,8 @@ main (int argc, char *argv[]) fprintf (stderr, "Error: listen() failed: %s\n", strerror (errno)); close (sock); - return 1; + ret = 1; + goto DONE; } if (background) { @@ -210,13 +217,15 @@ main (int argc, char *argv[]) printf ("smtp_dummy_pid='%d'\n", pid); fflush (stdout); close (sock); - return 0; + ret = 0; + goto DONE; } if (pid < 0) { fprintf (stderr, "Error: fork() failed: %s\n", strerror (errno)); close (sock); - return 1; + ret = 1; + goto DONE; } /* Reached if pid == 0 (the child process). */ /* Close stdout so that the one interested in pid value will @@ -239,21 +248,27 @@ main (int argc, char *argv[]) if (peer == -1) { fprintf (stderr, "Error: accept() failed: %s\n", strerror (errno)); - return 1; + ret = 1; + goto DONE; } peer_file = fdopen (peer, "w+"); if (peer_file == NULL) { fprintf (stderr, "Error: fdopen() failed: %s\n", strerror (errno)); - return 1; + ret = 1; + goto DONE; } do_smtp_to_file (peer_file, output); - fclose (output); - fclose (peer_file); - close (sock); + DONE: + if (output) + fclose (output); + if (peer_file) + fclose (peer_file); + if (sock >= 0) + close (sock); - return 0; + return ret; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-08-30 23:28 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-26 14:41 make notmuch cppcheck clean David Bremner 2017-08-26 14:41 ` [PATCH 1/5] build: add target to run cppcheck David Bremner 2017-08-27 8:20 ` Jani Nikula 2017-08-29 11:35 ` [Patch v2] " David Bremner 2017-08-29 18:36 ` Jani Nikula 2017-08-29 18:47 ` Jani Nikula 2017-08-30 23:28 ` David Bremner 2017-08-26 14:41 ` [PATCH 2/5] cppcheck: close files during shutdown David Bremner 2017-08-26 14:41 ` [PATCH 3/5] cppcheck: call va_end in _internal_error David Bremner 2017-08-26 14:41 ` [PATCH 4/5] test/smtp-dummy: uncrustify David Bremner 2017-08-26 14:41 ` [PATCH 5/5] test/smtp-dummy: convert to 'goto DONE' style David Bremner
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).