git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Makefile: detect errors in running spatch
@ 2017-03-10  8:31 Jeff King
  2017-03-10 17:03 ` René Scharfe
  2017-03-10 20:16 ` Junio C Hamano
  0 siblings, 2 replies; 5+ messages in thread
From: Jeff King @ 2017-03-10  8:31 UTC (permalink / raw)
  To: René Scharfe; +Cc: git

The "make coccicheck" target runs spatch against each source
file. But it does so in a for loop, so "make" never sees the
exit code of spatch. Worse, it redirects stderr to a log
file, so the user has no indication of any failure. And then
to top it all off, because we touched the patch file's
mtime, make will refuse to repeat the command because it
think the target is up-to-date.

So for example:

  $ make coccicheck SPATCH=does-not-exist
      SPATCH contrib/coccinelle/free.cocci
      SPATCH contrib/coccinelle/qsort.cocci
      SPATCH contrib/coccinelle/xstrdup_or_null.cocci
      SPATCH contrib/coccinelle/swap.cocci
      SPATCH contrib/coccinelle/strbuf.cocci
      SPATCH contrib/coccinelle/object_id.cocci
      SPATCH contrib/coccinelle/array.cocci
  $ make coccicheck SPATCH=does-not-exist
  make: Nothing to be done for 'coccicheck'.

With this patch, you get:

  $ make coccicheck SPATCH=does-not-exist
       SPATCH contrib/coccinelle/free.cocci
  /bin/sh: 4: does-not-exist: not found
  Makefile:2338: recipe for target 'contrib/coccinelle/free.cocci.patch' failed
  make: *** [contrib/coccinelle/free.cocci.patch] Error 1

It also dumps the log on failure, so any errors from spatch
itself (like syntax errors in our .cocci files) will be seen
by the user.

Signed-off-by: Jeff King <peff@peff.net>
---
This shell code is getting a bit unwieldy to stick inside the Makefile,
with all the line continuation and $-escaping.  It might be worth moving
it into a helper script.

It also doesn't help that shells are awkward at passing status out of a
for-loop. I think the most "make-ish" way of doing this would actually
be to lose the for loop and have a per-cocci-per-source target.

I don't know if that would make the patches harder to apply. The results
aren't full patches, so I assume you usually do some kind of munging on
them? I resorted to:

  make coccicheck SPATCH='spatch --in-place'

 Makefile | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 9ec6065cc..d97633892 100644
--- a/Makefile
+++ b/Makefile
@@ -2336,9 +2336,17 @@ check: common-cmds.h
 C_SOURCES = $(patsubst %.o,%.c,$(C_OBJ))
 %.cocci.patch: %.cocci $(C_SOURCES)
 	@echo '    ' SPATCH $<; \
+	ret=0; \
 	for f in $(C_SOURCES); do \
-		$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS); \
-	done >$@ 2>$@.log; \
+		$(SPATCH) --sp-file $< $$f $(SPATCH_FLAGS) || \
+			{ ret=$$?; break; }; \
+	done >$@+ 2>$@.log; \
+	if test $$ret != 0; \
+	then \
+		cat $@.log; \
+		exit 1; \
+	fi; \
+	mv $@+ $@; \
 	if test -s $@; \
 	then \
 		echo '    ' SPATCH result: $@; \
-- 
2.12.0.450.gd7e60cc16

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 0/18] snprintf cleanups
@ 2017-03-28 19:42 Jeff King
  2017-03-29  7:10 ` [PATCH] Makefile: detect errors in running spatch Jeff King
  0 siblings, 1 reply; 5+ messages in thread
From: Jeff King @ 2017-03-28 19:42 UTC (permalink / raw)
  To: git

Our code base calls snprintf() into a fixed-size buffer in a bunch of
places. Sometimes we check the result, and sometimes we accept a silent
truncation. In some cases an overflow is easy given long input. In some
cases it's impossible. And in some cases it depends on how big PATH_MAX
is on your filesystem, and whether it's actually enforced. :)

This series attempts to give more predictable and consistent results by
removing arbitrary buffer limitations. It also tries to make further
audits of snprintf() easier by converting to xsnprintf() where
appropriate.

There are still some snprintf() calls left after this. A few are in code
that's in flux, or is being cleaned up in nearby series (several of my
recent cleanup series were split off from this). A few should probably
remain (e.g., git-daemon will refuse to consider a repo name larger than
PATH_MAX, which may be a reasonable defense against weird memory tricks.
I wouldn't be sad to see this turned into a strbuf with an explicit
length policy enforced separately, though). And there were a few that I
just didn't get around to converting (the dumb-http walker, for example,
but I think it may need a pretty involved audit overall).

It's a lot of patches, but hopefully they're all pretty straightforward
to read.

  [01/18]: do not check odb_mkstemp return value for errors
  [02/18]: odb_mkstemp: write filename into strbuf
  [03/18]: odb_mkstemp: use git_path_buf
  [04/18]: diff: avoid fixed-size buffer for patch-ids
  [05/18]: tag: use strbuf to format tag header
  [06/18]: fetch: use heap buffer to format reflog
  [07/18]: avoid using fixed PATH_MAX buffers for refs
  [08/18]: avoid using mksnpath for refs
  [09/18]: create_branch: move msg setup closer to point of use
  [10/18]: create_branch: use xstrfmt for reflog message
  [11/18]: name-rev: replace static buffer with strbuf
  [12/18]: receive-pack: print --pack-header directly into argv array
  [13/18]: replace unchecked snprintf calls with heap buffers
  [14/18]: combine-diff: replace malloc/snprintf with xstrfmt
  [15/18]: convert unchecked snprintf into xsnprintf
  [16/18]: transport-helper: replace checked snprintf with xsnprintf
  [17/18]: gc: replace local buffer with git_path
  [18/18]: daemon: use an argv_array to exec children

 bisect.c               |  8 +++++---
 branch.c               | 16 ++++++++--------
 builtin/checkout.c     |  5 ++---
 builtin/fetch.c        |  6 ++++--
 builtin/gc.c           |  8 +-------
 builtin/index-pack.c   | 22 ++++++++++++----------
 builtin/ls-remote.c    | 10 ++++++----
 builtin/name-rev.c     | 21 ++++++++++++---------
 builtin/notes.c        |  9 ++++-----
 builtin/receive-pack.c | 17 ++++++++++-------
 builtin/replace.c      | 50 +++++++++++++++++++++++++++-----------------------
 builtin/rev-parse.c    |  5 +++--
 builtin/tag.c          | 42 ++++++++++++++++++------------------------
 cache.h                |  7 +++++--
 combine-diff.c         |  7 ++++---
 daemon.c               | 38 +++++++++++++++++---------------------
 diff.c                 | 20 +++++++++++++-------
 environment.c          | 14 ++++++--------
 fast-import.c          |  9 +++++----
 grep.c                 |  4 ++--
 http.c                 | 10 +++++-----
 imap-send.c            |  2 +-
 pack-bitmap-write.c    | 14 +++++++-------
 pack-write.c           | 16 ++++++++--------
 refs.c                 | 44 ++++++++++++++++++++++++++------------------
 sha1_file.c            |  4 ++--
 submodule.c            |  2 +-
 transport-helper.c     |  5 +----
 28 files changed, 215 insertions(+), 200 deletions(-)

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-29  7:11 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-10  8:31 [PATCH] Makefile: detect errors in running spatch Jeff King
2017-03-10 17:03 ` René Scharfe
2017-03-10 18:12   ` Jeff King
2017-03-10 20:16 ` Junio C Hamano
  -- strict thread matches above, loose matches on Subject: below --
2017-03-28 19:42 [PATCH 0/18] snprintf cleanups Jeff King
2017-03-29  7:10 ` [PATCH] Makefile: detect errors in running spatch Jeff King

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 NNTP newsgroup(s).