[PATCH 0/7] pull: make smatch scan output easy to digest

[PATCH 0/7] pull: make smatch scan output easy to digest

[Sami Kerola]

Hello,

I have had habit running smatch agains the project occasionally, and output
has always been noisy to the point not seeing the forest for the trees. 
This pull attempts to fix some genuine issues found with smatch, and trivial
warnings so that there is a lot less to digest when doing static analysis.

Changes 0002, 0003, and 0004 fall into later category -- and that is the
only justification they stand for.  Admittedly changing 0's to NULL has no
technical difference, but I do hope having this change encourages more
regular smatching of the code.

p.s.  I chose to refer patches 0002 and 0003 to avoid sending 150kb emails
to everyone on list.  Please see them from my remote repository instead, the
patch mails has direct link to github or you can add my remote branch using
details below.

----------------------------------------------------------------
The following changes since commit b192dd6943e5bb5d2a3773b2c9b06cbd4eb28258:
  lib/randutils: glibc 2.25 has getrandom(2) declaration (2017-02-13 14:14:28 +0100)
are available in the git repository at:
  git://github.com/kerolasa/lelux-utiliteetit.git smatch-fixes
for you to fetch changes up to ffd1a3c942bfe4dbf59584954a4eb0bf09c6b89b:
  findmnt: fix couple memory leaks [cppcheck] (2017-02-13 21:19:52 +0000)
----------------------------------------------------------------

Sami Kerola (7):
  cfdisk: avoid use of VLA in combination with sizeof() [smatch scan]
  misc: do not use plain 0 as NULL [smatch scan]
  misc: add static keyword to where needed [smatch scan]
  misc: unify function declarations [smatch scan]
  lib/idcache: add void to function declaration [smatch scan]
  libblkid: declare across file variables in blkidP.h [smatch scan]
  findmnt: fix couple memory leaks [cppcheck]

 Documentation/boilerplate.c           |  18 +++---
 disk-utils/addpart.c                  |   6 +-
 disk-utils/cfdisk.c                   |  15 +++--
 disk-utils/delpart.c                  |   6 +-
 disk-utils/fdformat.c                 |   2 +-
 disk-utils/fdisk-menu.c               |  16 ++---
 disk-utils/fsck.c                     |  26 ++++----
 disk-utils/fsck.cramfs.c              |  22 +++----
 disk-utils/isosize.c                  |  10 +--
 disk-utils/mkfs.cramfs.c              |   6 +-
 disk-utils/mkswap.c                   |  18 +++---
 disk-utils/partx.c                    |   4 +-
 disk-utils/raw.c                      |  18 +++---
 disk-utils/resizepart.c               |   6 +-
 disk-utils/sfdisk.c                   |   4 +-
 disk-utils/swaplabel.c                |  10 +--
 include/pt-mbr-partnames.h            |   2 +-
 lib/colors.c                          |   2 +-
 lib/exec_shell.c                      |   2 +-
 lib/idcache.c                         |   2 +-
 lib/loopdev.c                         |   2 +-
 lib/randutils.c                       |   4 +-
 lib/timer.c                           |   2 +-
 libblkid/src/blkidP.h                 |   5 ++
 libblkid/src/dev.c                    |   6 +-
 libblkid/src/devname.c                |  12 ++--
 libblkid/src/probe.c                  |   5 --
 libblkid/src/read.c                   |   6 +-
 libblkid/src/resolve.c                |   2 +-
 libblkid/src/superblocks/drbdmanage.c |   2 +-
 libblkid/src/superblocks/vfat.c       |   2 +-
 libblkid/src/tag.c                    |  12 ++--
 libblkid/src/topology/evms.c          |   2 +-
 libblkid/src/verify.c                 |   4 +-
 libfdisk/src/bsd.c                    |   2 +-
 libfdisk/src/context.c                |   4 +-
 libfdisk/src/sun.c                    |   2 +-
 libmount/python/context.c             |  46 +++++++-------
 libmount/python/fs.c                  |  46 +++++++-------
 libmount/python/pylibmount.c          |   2 +-
 libmount/python/tab.c                 |  46 +++++++-------
 libuuid/src/gen_uuid.c                |  10 +--
 login-utils/chfn.c                    |  14 ++---
 login-utils/chsh.c                    |  12 ++--
 login-utils/nologin.c                 |   6 +-
 login-utils/su-common.c               |   9 ++-
 login-utils/sulogin.c                 |  22 +++----
 login-utils/utmpdump.c                |  12 ++--
 login-utils/vipw.c                    |  11 ++--
 misc-utils/blkid.c                    |   2 +-
 misc-utils/cal.c                      |   4 +-
 misc-utils/findmnt-verify.c           |  10 +--
 misc-utils/findmnt.c                  |  78 +++++++++++------------
 misc-utils/getopt.c                   |   4 +-
 misc-utils/kill.c                     |   2 +-
 misc-utils/look.c                     |  18 +++---
 misc-utils/lsblk.c                    |  64 +++++++++----------
 misc-utils/namei.c                    |  18 +++---
 misc-utils/whereis.c                  |   2 +-
 misc-utils/wipefs.c                   |  22 +++----
 schedutils/ionice.c                   |   2 +-
 sys-utils/blkdiscard.c                |  18 +++---
 sys-utils/chcpu.c                     |  18 +++---
 sys-utils/eject.c                     |   4 +-
 sys-utils/fallocate.c                 |  26 ++++----
 sys-utils/flock.c                     |   2 +-
 sys-utils/fsfreeze.c                  |  10 +--
 sys-utils/fstrim.c                    |  16 ++---
 sys-utils/hwclock-cmos.c              |  26 ++++----
 sys-utils/hwclock.c                   |  71 +++++++++++----------
 sys-utils/hwclock.h                   |   7 ++-
 sys-utils/ipcs.c                      |   2 +-
 sys-utils/losetup.c                   |  44 ++++++-------
 sys-utils/lscpu.c                     |  36 +++++------
 sys-utils/lsipc.c                     |  42 ++++++-------
 sys-utils/lsns.c                      |   4 +-
 sys-utils/mount.c                     |  68 ++++++++++----------
 sys-utils/mountpoint.c                |  14 ++---
 sys-utils/prlimit.c                   |   2 +-
 sys-utils/readprofile.c               |   6 +-
 sys-utils/renice.c                    |   2 +-
 sys-utils/rtcwake.c                   |  32 +++++-----
 sys-utils/setpriv.c                   |  44 ++++++-------
 sys-utils/swapoff.c                   |  10 +--
 sys-utils/swapon.c                    |  34 +++++-----
 sys-utils/tunelp.c                    |  22 +++----
 sys-utils/umount.c                    |  34 +++++-----
 sys-utils/unshare.c                   |  34 +++++-----
 term-utils/agetty.c                   |  86 ++++++++++++-------------
 term-utils/mesg.c                     |   8 +--
 term-utils/script.c                   |   2 +-
 term-utils/setterm.c                  |   2 +-
 term-utils/wall.c                     |  12 ++--
 text-utils/col.c                      |  34 +++++-----
 text-utils/colcrt.c                   |   2 +-
 text-utils/colrm.c                    |   6 +-
 text-utils/column.c                   |  22 +++----
 text-utils/more.c                     | 115 +++++++++++++++++-----------------
 text-utils/pg.c                       |  64 +++++++++----------
 text-utils/rev.c                      |   8 +--
 text-utils/tailf.c                    |  10 +--
 text-utils/ul.c                       |  55 ++++++++--------
 102 files changed, 895 insertions(+), 890 deletions(-)

-- 
2.11.1

[PATCH 1/7] cfdisk: avoid use of VLA in combination with sizeof() [smatch scan]

[Sami Kerola]

disk-utils/cfdisk.c:1066:29: error: cannot size expression

One should use sizeof() only when variable size can be known at time of
compilation.  That is not the case with variable length arrays.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/cfdisk.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/disk-utils/cfdisk.c b/disk-utils/cfdisk.c
index 281d66c78..ce49a791c 100644
--- a/disk-utils/cfdisk.c
+++ b/disk-utils/cfdisk.c
@@ -1044,9 +1044,10 @@ static void ui_draw_menuitem(struct cfdisk *cf,
 			     struct cfdisk_menuitem *d,
 			     size_t idx)
 {
-	char buf[80 * MB_CUR_MAX], *ptr = buf;
+	char *buf, *ptr;
 	const char *name;
 	size_t width;
+	const size_t buf_sz = 80 * MB_CUR_MAX;
 	int ln, cl, vert = cf->menu->vertical;
 
 	if (!menuitem_on_page(cf, idx))
@@ -1054,6 +1055,7 @@ static void ui_draw_menuitem(struct cfdisk *cf,
 	ln = menuitem_get_line(cf, idx);
 	cl = menuitem_get_column(cf, idx);
 
+	ptr = buf = xmalloc(buf_sz);
 	/* string width */
 	if (vert) {
 		width = cf->menu->width + MENU_V_SPADDING;
@@ -1063,7 +1065,7 @@ static void ui_draw_menuitem(struct cfdisk *cf,
 		width = MENU_H_SPADDING + cf->menu->width + MENU_H_SPADDING;
 
 	name = _(d->name);
-	mbsalign(name, ptr, sizeof(buf), &width,
+	mbsalign(name, ptr, buf_sz, &width,
 			vert ? MBS_ALIGN_LEFT : MBS_ALIGN_CENTER,
 			0);
 
@@ -1082,6 +1084,7 @@ static void ui_draw_menuitem(struct cfdisk *cf,
 		mvprintw(ln, cl, "%s", buf);
 	else
 		mvprintw(ln, cl, "%s%s%s", MENU_H_PRESTR, buf, MENU_H_POSTSTR);
+	free(buf);
 
 	if (cf->menu->idx == idx) {
 		standend();
-- 
2.11.1

[PATCH 1/7] cfdisk: avoid use of VLA in combination with sizeof() [smatch scan]

[Sami Kerola]

From: Sami Kerola <kerolasa@iki.fi>
Date: Sat, 11 Feb 2017 20:23:26 +0000
Subject: [PATCH 2/7] misc: do not use plain 0 as NULL [smatch scan]
Organization: Cloudflare

text-utils/tailf.c:69:21: warning: Using plain integer as NULL pointer

Since many 'struct option' has used zero as NULL make them more readable in
same go by reindenting, and using named argument requirements.

Reference: https://lwn.net/Articles/93577/
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---

This is a big patch, and best to review either by looking with web browser
or pulling my remote branch.

https://github.com/kerolasa/lelux-utiliteetit/commit/0adeca850a8ce0d8ce9c638339008ee688c6fb09

-- 
2.11.1

[PATCH 3/7] misc: add static keyword to where needed [smatch scan]

[Sami Kerola]

text-utils/rev.c:68:9: warning: symbol 'buf' was not declared. Should it be
static?

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---

This is a big patch, and best to review either by looking with web browser
or pulling my remote branch.

https://github.com/kerolasa/lelux-utiliteetit/commit/f88ae748a761f0303589e3a6995d538c9fad2e09

-- 
2.11.1

[PATCH 4/7] misc: unify function declarations [smatch scan]

[Sami Kerola]

This will fix errors similar to below.

lib/exec_shell.c:33:6: error: symbol 'exec_shell' redeclared with different
type (originally declared at ./include/exec_shell.h:1) - different modifiers

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 lib/exec_shell.c        |  2 +-
 login-utils/su-common.c |  5 ++---
 login-utils/vipw.c      |  5 ++---
 sys-utils/hwclock.c     |  3 +--
 sys-utils/hwclock.h     |  7 ++++++-
 term-utils/agetty.c     | 16 ++++++++--------
 text-utils/more.c       |  7 ++++---
 7 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/lib/exec_shell.c b/lib/exec_shell.c
index a7374bd33..481184394 100644
--- a/lib/exec_shell.c
+++ b/lib/exec_shell.c
@@ -30,7 +30,7 @@
 
 #define DEFAULT_SHELL "/bin/sh"
 
-void exec_shell(void)
+__attribute__((__noreturn__)) void exec_shell(void)
 {
 	const char *shell = getenv("SHELL");
 	char *shellc = xstrdup(shell);
diff --git a/login-utils/su-common.c b/login-utils/su-common.c
index 41f2b1cea..3660ed1c8 100644
--- a/login-utils/su-common.c
+++ b/login-utils/su-common.c
@@ -97,8 +97,7 @@ enum
 extern char **environ;
 #endif
 
-static void run_shell (char const *, char const *, char **, size_t)
-     __attribute__ ((__noreturn__));
+static __attribute__ ((__noreturn__)) void run_shell (char const *, char const *, char **, size_t);
 
 /* If true, pass the `-f' option to the subshell.  */
 static bool fast_startup;
@@ -612,7 +611,7 @@ change_identity (const struct passwd *pw)
    Pass ADDITIONAL_ARGS to the shell as more arguments; there
    are N_ADDITIONAL_ARGS extra arguments.  */
 
-static void
+static __attribute__ ((__noreturn__)) void
 run_shell (char const *shell, char const *command, char **additional_args,
 	   size_t n_additional_args)
 {
diff --git a/login-utils/vipw.c b/login-utils/vipw.c
index 4d8592434..58d72584f 100644
--- a/login-utils/vipw.c
+++ b/login-utils/vipw.c
@@ -85,7 +85,7 @@ static int program;
 static char orig_file[FILENAMELEN];	/* original file /etc/passwd or /etc/group */
 static char *tmp_file;			/* tmp file */
 
-void pw_error (char *, int, int);
+__attribute__((__noreturn__)) void pw_error(char *, int, int);
 
 static void copyfile(int from, int to)
 {
@@ -224,8 +224,7 @@ static void pw_edit(void)
 	free(editor);
 }
 
-void __attribute__((__noreturn__))
-pw_error(char *name, int err, int eval)
+__attribute__((__noreturn__)) void pw_error(char *name, int err, int eval)
 {
 	if (err) {
 		if (name)
diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 21d1304e1..0631f4d51 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1738,8 +1738,7 @@ int main(int argc, char **argv)
 	return rc;		/* Not reached */
 }
 
-void __attribute__((__noreturn__))
-hwclock_exit(const struct hwclock_control *ctl
+__attribute__((__noreturn__)) void hwclock_exit(const struct hwclock_control *ctl
 #ifndef HAVE_LIBAUDIT
 	     __attribute__((__unused__))
 #endif
diff --git a/sys-utils/hwclock.h b/sys-utils/hwclock.h
index b08bd2d7a..6fa78a14e 100644
--- a/sys-utils/hwclock.h
+++ b/sys-utils/hwclock.h
@@ -80,6 +80,11 @@ extern void set_cmos_access(const struct hwclock_control *ctl);
 extern int get_epoch_rtc(const struct hwclock_control *ctl, unsigned long *epoch, int silent);
 extern int set_epoch_rtc(const struct hwclock_control *ctl);
 
-extern void hwclock_exit(const struct hwclock_control *ctl, int status);
+extern __attribute__((__noreturn__))
+void hwclock_exit(const struct hwclock_control *ctl,
+#ifndef HAVE_LIBAUDIT
+		  __attribute__((__unused__))
+#endif
+		  int status);
 
 #endif				/* HWCLOCK_CLOCK_H */
diff --git a/term-utils/agetty.c b/term-utils/agetty.c
index 8ac08f57c..fa86ce419 100644
--- a/term-utils/agetty.c
+++ b/term-utils/agetty.c
@@ -308,11 +308,10 @@ static void termio_final(struct options *op,
 			 struct termios *tp, struct chardata *cp);
 static int caps_lock(char *s);
 static speed_t bcode(char *s);
-static void usage(FILE * out) __attribute__((__noreturn__));
-static void log_err(const char *, ...) __attribute__((__noreturn__))
-			       __attribute__((__format__(printf, 1, 2)));
-static void log_warn (const char *, ...)
-				__attribute__((__format__(printf, 1, 2)));
+static __attribute__ ((__noreturn__)) void usage(FILE *out);
+static __attribute__((__noreturn__)) __attribute__((__format__(printf, 1, 2)))
+       void log_err(const char *fmt, ...);
+static __attribute__((__format__(printf, 1, 2))) void log_warn (const char *, ...);
 static ssize_t append(char *dest, size_t len, const char  *sep, const char *src);
 static void check_username (const char* nm);
 static void login_options_to_argv(char *argv[], int *argc, char *str, char *username);
@@ -2086,7 +2085,7 @@ static speed_t bcode(char *s)
 	return 0;
 }
 
-static void __attribute__ ((__noreturn__)) usage(FILE *out)
+static __attribute__ ((__noreturn__))void  usage(FILE *out)
 {
 	fputs(USAGE_HEADER, out);
 	fprintf(out, _(" %1$s [options] <line> [<baud_rate>,...] [<termtype>]\n"
@@ -2182,7 +2181,8 @@ static void dolog(int priority, const char *fmt, va_list ap)
 #endif				/* USE_SYSLOG */
 }
 
-static void log_err(const char *fmt, ...)
+static __attribute__((__noreturn__)) __attribute__((__format__(printf, 1, 2)))
+void log_err(const char *fmt, ...)
 {
 	va_list ap;
 
@@ -2195,7 +2195,7 @@ static void log_err(const char *fmt, ...)
 	exit(EXIT_FAILURE);
 }
 
-static void log_warn(const char *fmt, ...)
+static __attribute__((__format__(printf, 1, 2))) void log_warn(const char *fmt, ...)
 {
 	va_list ap;
 
diff --git a/text-utils/more.c b/text-utils/more.c
index d4aba11e4..509e746d4 100644
--- a/text-utils/more.c
+++ b/text-utils/more.c
@@ -140,7 +140,8 @@ static struct termios otty, savetty0;
 static long file_pos, file_size;
 static int fnum, no_intty, no_tty, slow_tty;
 static int dum_opt, dlines;
-static void onquit(int), onsusp(int), chgwinsz(int), end_it(int);
+static void onquit(int), onsusp(int), chgwinsz(int);
+static __attribute__((__noreturn__)) void end_it(int dummy __attribute__((__unused__)));
 static int nscroll = SCROLL_LEN;	/* Number of lines scrolled by 'd' */
 static int fold_opt = 1;		/* Fold long lines */
 static int stop_opt = 1;		/* Stop after form feeds */
@@ -219,7 +220,7 @@ static void putstring(char *s)
 	tputs(s, fileno(stdout), putchar);	/* putp(s); */
 }
 
-static void __attribute__((__noreturn__)) usage(FILE *out)
+static __attribute__((__noreturn__)) void usage(FILE *out)
 {
 	fputs(USAGE_HEADER, out);
 	fprintf(out, _(" %s [options] <file>...\n"), program_invocation_short_name);
@@ -2101,7 +2102,7 @@ void rdline(register FILE *f)
 }
 
 /* Come here when we get a suspend signal from the terminal */
-void onsusp(int dummy __attribute__((__unused__)))
+static void onsusp(int dummy __attribute__((__unused__)))
 {
 	sigset_t signals, oldmask;
 
-- 
2.11.1

[PATCH 5/7] lib/idcache: add void to function declaration [smatch scan]

[Sami Kerola]

lib/idcache.c:29:29: warning: non-ANSI function declaration of function
'new_idcache'

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 lib/idcache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/idcache.c b/lib/idcache.c
index 3c358b8a8..a1a957b93 100644
--- a/lib/idcache.c
+++ b/lib/idcache.c
@@ -26,7 +26,7 @@ struct identry *get_id(struct idcache *ic, unsigned long int id)
 	return NULL;
 }
 
-struct idcache *new_idcache()
+struct idcache *new_idcache(void)
 {
 	return calloc(1, sizeof(struct idcache));
 }
-- 
2.11.1

[PATCH 6/7] libblkid: declare across file variables in blkidP.h [smatch scan]

[Sami Kerola]

Else these variables will cause following warning:

libblkid/src/superblocks/superblocks.c:165:29: warning: symbol
'superblocks_drv' was not declared.  Should it be static?

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 libblkid/src/blkidP.h | 5 +++++
 libblkid/src/probe.c  | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/libblkid/src/blkidP.h b/libblkid/src/blkidP.h
index 817c8b02b..de41473a8 100644
--- a/libblkid/src/blkidP.h
+++ b/libblkid/src/blkidP.h
@@ -116,6 +116,11 @@ struct blkid_chaindrv {
 	void		(*free_data)(blkid_probe, void *);
 };
 
+/* chains */
+extern const struct blkid_chaindrv superblocks_drv;
+extern const struct blkid_chaindrv topology_drv;
+extern const struct blkid_chaindrv partitions_drv;
+
 /*
  * Low-level probe result
  */
diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
index df90a82e0..ab05fb2d3 100644
--- a/libblkid/src/probe.c
+++ b/libblkid/src/probe.c
@@ -115,11 +115,6 @@
 #include "strutils.h"
 #include "list.h"
 
-/* chains */
-extern const struct blkid_chaindrv superblocks_drv;
-extern const struct blkid_chaindrv topology_drv;
-extern const struct blkid_chaindrv partitions_drv;
-
 /*
  * All supported chains
  */
-- 
2.11.1

[PATCH 7/7] findmnt: fix couple memory leaks [cppcheck]

[Sami Kerola]

In both cases FILE *f was leaked.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/findmnt-verify.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/misc-utils/findmnt-verify.c b/misc-utils/findmnt-verify.c
index b2a88c5f6..e3fb391d4 100644
--- a/misc-utils/findmnt-verify.c
+++ b/misc-utils/findmnt-verify.c
@@ -328,14 +328,15 @@ static int read_proc_filesystems(struct verify_context *vfy)
 
 		rc = add_filesystem(vfy, cp);
 		if (rc)
-			return rc;
+			break;
 	}
 	fclose(f);
-	return 0;
+	return rc;
 }
 
 static int read_kernel_filesystems(struct verify_context *vfy)
 {
+	int rc = 0;
 #ifdef __linux__
 	struct utsname uts;
 	FILE *f;
@@ -351,7 +352,6 @@ static int read_kernel_filesystems(struct verify_context *vfy)
 
 	while (!feof(f)) {
 		char *p, *name;
-		int rc;
 
 		if (!fgets(buf, sizeof(buf), f))
 			break;
@@ -377,11 +377,11 @@ static int read_kernel_filesystems(struct verify_context *vfy)
 
 		rc = add_filesystem(vfy, name);
 		if (rc)
-			return rc;
+			break;
 	}
 	fclose(f);
 #endif /* __linux__ */
-	return 0;
+	return rc;
 }
 
 static int verify_fstype(struct verify_context *vfy)
-- 
2.11.1

Re: [PATCH 0/7] pull: make smatch scan output easy to digest

[Karel Zak]

Merged, but except:

On Mon, Feb 13, 2017 at 10:06:34PM +0000, Sami Kerola wrote:
>   misc: unify function declarations [smatch scan]

I agree that some unification would be nice, but don't like

   static __attribute__((__noreturn__)) __attribute__((__format__(printf, 1, 2)))
          void log_err(const char *fmt, ...);

it would be better to keep all the attributes after declaration:

   static void log_err(const char *fmt, ...)
                    __attribute__((__noreturn__))
                    __attribute__((__format__(printf, 1, 2)));

it means new line for each attribute. IMHO it's more readable and you
don't have to search for the function name, etc.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/7] pull: make smatch scan output easy to digest

[Sami Kerola]

On 20 February 2017 at 12:14, Karel Zak <kzak@redhat.com> wrote:
> Merged, but except:
>
> On Mon, Feb 13, 2017 at 10:06:34PM +0000, Sami Kerola wrote:
>>   misc: unify function declarations [smatch scan]
>
> I agree that some unification would be nice, but don't like
>
>    static __attribute__((__noreturn__)) __attribute__((__format__(printf, 1, 2)))
>           void log_err(const char *fmt, ...);
>
> it would be better to keep all the attributes after declaration:
>
>    static void log_err(const char *fmt, ...)
>                     __attribute__((__noreturn__))
>                     __attribute__((__format__(printf, 1, 2)));
>
> it means new line for each attribute. IMHO it's more readable and you
> don't have to search for the function name, etc.

Hi Karel,

That's fair enough, and truth is the patch did not fix many warnings so
I am sure we can live with them.

What comes to moving attributes at end of the declaration that does
not please smatch.

lib/exec_shell.c:33:1: error: attributes should be specified before
the declarator in a function definition
 void exec_shell(void)

I tried to look docs if attribute position really has some special meaning,
but could not find anything. Before digging deeper with this perhaps
someone on the list can tell what is going on with this warning.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 0/7] pull: make smatch scan output easy to digest

[Karel Zak]

On Mon, Feb 20, 2017 at 10:12:06PM +0000, Sami Kerola wrote:
> On 20 February 2017 at 12:14, Karel Zak <kzak@redhat.com> wrote:
> > Merged, but except:
> >
> > On Mon, Feb 13, 2017 at 10:06:34PM +0000, Sami Kerola wrote:
> >>   misc: unify function declarations [smatch scan]
> >
> > I agree that some unification would be nice, but don't like
> >
> >    static __attribute__((__noreturn__)) __attribute__((__format__(printf, 1, 2)))
> >           void log_err(const char *fmt, ...);
> >
> > it would be better to keep all the attributes after declaration:
> >
> >    static void log_err(const char *fmt, ...)
> >                     __attribute__((__noreturn__))
> >                     __attribute__((__format__(printf, 1, 2)));
> >
> > it means new line for each attribute. IMHO it's more readable and you
> > don't have to search for the function name, etc.
> 
> Hi Karel,
> 
> That's fair enough, and truth is the patch did not fix many warnings so
> I am sure we can live with them.
> 
> What comes to moving attributes at end of the declaration that does
> not please smatch.
> 
> lib/exec_shell.c:33:1: error: attributes should be specified before
> the declarator in a function definition
>  void exec_shell(void)

Report it as smatch bug. It's pretty common that function attributes
are defined after declaration. CC: Dan.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/7] pull: make smatch scan output easy to digest

[Dan Carpenter]

On Tue, Feb 21, 2017 at 10:50:12AM +0100, Karel Zak wrote:
> On Mon, Feb 20, 2017 at 10:12:06PM +0000, Sami Kerola wrote:
> > On 20 February 2017 at 12:14, Karel Zak <kzak@redhat.com> wrote:
> > > Merged, but except:
> > >
> > > On Mon, Feb 13, 2017 at 10:06:34PM +0000, Sami Kerola wrote:
> > >>   misc: unify function declarations [smatch scan]
> > >
> > > I agree that some unification would be nice, but don't like
> > >
> > >    static __attribute__((__noreturn__)) __attribute__((__format__(printf, 1, 2)))
> > >           void log_err(const char *fmt, ...);
> > >
> > > it would be better to keep all the attributes after declaration:
> > >
> > >    static void log_err(const char *fmt, ...)
> > >                     __attribute__((__noreturn__))
> > >                     __attribute__((__format__(printf, 1, 2)));
> > >
> > > it means new line for each attribute. IMHO it's more readable and you
> > > don't have to search for the function name, etc.
> > 
> > Hi Karel,
> > 
> > That's fair enough, and truth is the patch did not fix many warnings so
> > I am sure we can live with them.
> > 
> > What comes to moving attributes at end of the declaration that does
> > not please smatch.
> > 
> > lib/exec_shell.c:33:1: error: attributes should be specified before
> > the declarator in a function definition
> >  void exec_shell(void)
> 
> Report it as smatch bug. It's pretty common that function attributes
> are defined after declaration. CC: Dan.
> 

This is a Sparse warning, not a Smatch warning.  It shouldn't show up
when you do a `make checksmatch` so I'm confused by that.  Anyway,
could you email linux-sparse@vger.kernel.org?

regards,
dan carpenter

Re: [PATCH 0/7] pull: make smatch scan output easy to digest

[Sami Kerola]

On Fri, 24 Feb 2017, Dan Carpenter wrote:

> On Tue, Feb 21, 2017 at 10:50:12AM +0100, Karel Zak wrote:
> > On Mon, Feb 20, 2017 at 10:12:06PM +0000, Sami Kerola wrote:
> > > On 20 February 2017 at 12:14, Karel Zak <kzak@redhat.com> wrote:
> > > > Merged, but except:
> > > >
> > > > On Mon, Feb 13, 2017 at 10:06:34PM +0000, Sami Kerola wrote:
> > > >>   misc: unify function declarations [smatch scan]
> > > >
> > > > I agree that some unification would be nice, but don't like
> > > >
> > > >    static __attribute__((__noreturn__)) __attribute__((__format__(printf, 1, 2)))
> > > >           void log_err(const char *fmt, ...);
> > > >
> > > > it would be better to keep all the attributes after declaration:
> > > >
> > > >    static void log_err(const char *fmt, ...)
> > > >                     __attribute__((__noreturn__))
> > > >                     __attribute__((__format__(printf, 1, 2)));
> > > >
> > > > it means new line for each attribute. IMHO it's more readable and you
> > > > don't have to search for the function name, etc.
> > > 
> > > Hi Karel,
> > > 
> > > That's fair enough, and truth is the patch did not fix many warnings so
> > > I am sure we can live with them.
> > > 
> > > What comes to moving attributes at end of the declaration that does
> > > not please smatch.
> > > 
> > > lib/exec_shell.c:33:1: error: attributes should be specified before
> > > the declarator in a function definition
> > >  void exec_shell(void)
> > 
> > Report it as smatch bug. It's pretty common that function attributes
> > are defined after declaration. CC: Dan.
> > 
> 
> This is a Sparse warning, not a Smatch warning.  It shouldn't show up
> when you do a `make checksmatch` so I'm confused by that.  Anyway,
> could you email linux-sparse@vger.kernel.org?

Hi Dan, Karel, and others,

I did not run 'make checksmatch', but added following variables and did 
usual ./configure && make

export CC=/src/smatch/cgcc
export PATH="/src/smatch:$PATH"
export CFLAGS="-Wsparse-all"

With these right now upstream gives errors such as this:

text-utils/more.c:674:43: error: symbol 'end_it' redeclared with different 
type (originally declared at text-utils/more.c:143) - different modifiers

that are caused by lack of function attributes in declaration, while 
fuction has them. An attempt to fix the declarations caused made me to get 
noise about attributes vs declaration order. Such versions of files never 
entered to util-linux, and that is the reason why you do not see these.

See below simple source file that demonstrates how to triggers warning in 
question. When I said was 'smatch' printed warnings it was cgcc that was 
complaining , and therefore this is not really a 'smatch' problem at all. 
Sorry about giving wrong impression.

When comparing gcc with clang gives very similar warning. Perhaps the 
right way to go about this is to reorder attributes to be in front of 
functions after all.

-- snip --

$ cat ./test.c 
#include <stdio.h>
#include <stdlib.h>
static void hello(char *s) __attribute__((__noreturn__));
static void hello(char *s) __attribute__((__noreturn__)) {
        printf("hello %s\n", s);
        exit(0);
}
int main(int argc, char **argv) {
        if (1 < argc)
                hello(argv[1]);
        printf("hello world\n");
        return 0;
}


$ gcc ./test.c 
./test.c:4:1: error: attributes should be specified before the declarator in a function definition
 static void hello(char *s) __attribute__((__noreturn__)) {
 ^~~~~~
./test.c:3:13: warning: 'hello' used but never defined
 static void hello(char *s) __attribute__((__noreturn__));
             ^~~~~
$ gcc --version
gcc (GCC) 6.3.1 20170109
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


$ clang ./test.c 
./test.c:4:43: warning: GCC does not allow '__noreturn__' attribute in this position on a function
      definition [-Wgcc-compat]
static void hello(char *s) __attribute__((__noreturn__)) {
                                          ^
1 warning generated.
$ clang --version
clang version 3.9.1 (tags/RELEASE_391/final)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin


See also:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60915
https://gcc.gnu.org/onlinedocs/gcc-4.2.2/gcc/Attribute-Syntax.html

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 0/7] pull: make smatch scan output easy to digest

[Dan Carpenter]

On Sat, Feb 25, 2017 at 05:32:23PM +0000, Sami Kerola wrote:
> See below simple source file that demonstrates how to triggers warning in 
> question. When I said was 'smatch' printed warnings it was cgcc that was 
> complaining , and therefore this is not really a 'smatch' problem at all. 
> Sorry about giving wrong impression.

cgcc is just a shell script which calls either smatch or sparse.  Smatch
uses sparse as a parser so you sometimes do see certain sparse messages
when you run smatch, but I don't do bug fixes for those normally.  You
would think I would understand sparse really well because I use it so
much but actually hacking sparse makes me realize I'm kind of dumb.  ;P

regards,
dan carpenter

Re: [PATCH 0/7] pull: make smatch scan output easy to digest

[Luc Van Oostenryck]

On Sat, Feb 25, 2017 at 05:32:23PM +0000, Sami Kerola wrote:
> On Fri, 24 Feb 2017, Dan Carpenter wrote:
> text-utils/more.c:674:43: error: symbol 'end_it' redeclared with different 
> type (originally declared at text-utils/more.c:143) - different modifiers
> 
> that are caused by lack of function attributes in declaration, while 
> fuction has them.

Yes, this is a sparse's bug.
I hope I'll be able to look at it soon.

> See below simple source file that demonstrates how to triggers warning in 
> question. When I said was 'smatch' printed warnings it was cgcc that was 
> complaining , and therefore this is not really a 'smatch' problem at all. 
> Sorry about giving wrong impression.
> 
> When comparing gcc with clang gives very similar warning. Perhaps the 
> right way to go about this is to reorder attributes to be in front of 
> functions after all.


Luc Van Oostenryck