[PATCH 00/12] pull: mostly swap command clarifications

[PATCH 00/12] pull: mostly swap command clarifications

[Sami Kerola]

Hi Karel and others,

Seeing the swaplabel bug fix went to stable release I decided to clarify
swap related commands what comes to version.  Other than that these
changes include fix to a last(1) regression that I caused with the
is_phantom() function, commit itself has more info what went wrong
previously.  The rest is pretty random stuff.


The following changes since commit c425353570b3f8b429790ff8bbb959348eead233:

  docs: update TODO (2014-04-23 15:28:09 +0200)

are available in the git repository at:

  git://github.com/kerolasa/lelux-utiliteetit.git 2014wk16

for you to fetch changes up to 975da9cf6986fdcdb7207e24403322a265d76ed0:

  lib/pager: use names when referring to standard file descriptors (2014-04-26 18:30:06 +0100)

Sami Kerola (12):
  include/xalloc: ensure arithmetics overflow cannot happen
  dmesg: move get_boot_time() to lib/timeutils
  last: fix is_phantom() detection
  include/c.h: add macro to print definitions as string
  mkswap, swaplabel: move version number to header
  mkswap: remove legacy swap structure
  include/swapheader.h: ensure type sizes
  swapon: swaps with legacy version label are not supported
  swapon, swapheader, mkswap: move swap signature to header
  libsmartcols: remove ununsed assignment
  lib/timeutils: fix memory leak
  lib/pager: use names when referring to standard file descriptors

 disk-utils/mkfs.cramfs.c       |  2 +-
 disk-utils/mkswap.c            | 17 ++++-----
 disk-utils/swaplabel.c         |  4 +-
 include/c.h                    |  9 +++++
 include/swapheader.h           | 26 ++++++-------
 include/timeutils.h            |  1 +
 include/xalloc.h               | 85 +++++++++++++++++++++++++-----------------
 lib/pager.c                    | 18 ++++-----
 lib/timeutils.c                | 30 +++++++++++++++
 libsmartcols/src/table_print.c |  2 +-
 login-utils/last.c             | 15 ++++++--
 login-utils/login.c            |  2 +-
 misc-utils/blkid.c             |  2 +-
 misc-utils/findmnt.c           |  2 +-
 misc-utils/getopt.c            |  4 +-
 misc-utils/logger.c            |  2 +-
 sys-utils/dmesg.c              | 27 --------------
 sys-utils/swapon-common.c      |  4 +-
 sys-utils/swapon.c             | 16 +++-----
 text-utils/col.c               |  9 ++---
 text-utils/column.c            |  7 ++--
 text-utils/more.c              |  4 +-
 text-utils/rev.c               |  2 +-
 text-utils/ul.c                |  2 +-
 24 files changed, 156 insertions(+), 136 deletions(-)

-- 
1.9.2

[PATCH 01/12] include/xalloc: ensure arithmetics overflow cannot happen

[Sami Kerola]

The xrealloc() changes has the greatest change.  It splits the size and
multiplier arguments so that arithmetics overflow can be detected.  This
change is propagated to use of the function in other files.

Additionally this change checks that size inputs for allocations are
never zero.  It is uncertain if in these cases abort() should be called
to get a core.

The xstrdup() is made to use memcpy(), which is exactly what the library
call does so one layer of absraction is saved here.

References: http://undeadly.org/cgi?action=article&sid=20060330071917
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/mkfs.cramfs.c  |  2 +-
 include/xalloc.h          | 85 ++++++++++++++++++++++++++++-------------------
 login-utils/login.c       |  2 +-
 misc-utils/blkid.c        |  2 +-
 misc-utils/findmnt.c      |  2 +-
 misc-utils/getopt.c       |  4 +--
 misc-utils/logger.c       |  2 +-
 sys-utils/swapon-common.c |  4 +--
 text-utils/col.c          |  9 ++---
 text-utils/column.c       |  7 ++--
 text-utils/more.c         |  4 +--
 text-utils/rev.c          |  2 +-
 text-utils/ul.c           |  2 +-
 13 files changed, 69 insertions(+), 58 deletions(-)

diff --git a/disk-utils/mkfs.cramfs.c b/disk-utils/mkfs.cramfs.c
index ebc8112..7e4ee2b 100644
--- a/disk-utils/mkfs.cramfs.c
+++ b/disk-utils/mkfs.cramfs.c
@@ -497,7 +497,7 @@ static unsigned int write_directory_structure(struct entry *entry, char *base, u
 			if (entry->child) {
 				if (stack_entries >= stack_size) {
 					stack_size *= 2;
-					entry_stack = xrealloc(entry_stack, stack_size * sizeof(struct entry *));
+					entry_stack = xrealloc(entry_stack, stack_size, sizeof(struct entry *));
 				}
 				entry_stack[stack_entries] = entry;
 				stack_entries++;
diff --git a/include/xalloc.h b/include/xalloc.h
index f012fb2..478ebef 100644
--- a/include/xalloc.h
+++ b/include/xalloc.h
@@ -12,6 +12,7 @@
 
 #include <stdlib.h>
 #include <string.h>
+#include <stdint.h>
 
 #include "c.h"
 
@@ -22,62 +23,77 @@
 static inline __ul_alloc_size(1)
 void *xmalloc(const size_t size)
 {
-        void *ret = malloc(size);
+	void *ret;
 
-        if (!ret && size)
-                err(XALLOC_EXIT_CODE, "cannot allocate %zu bytes", size);
-        return ret;
+	if (size == 0)
+		err(XALLOC_EXIT_CODE, "xmalloc: zero size");
+	ret = malloc(size);
+	if (!ret)
+		err(XALLOC_EXIT_CODE, "xmalloc: cannot allocate %zu bytes", size);
+	return ret;
 }
 
 static inline __ul_alloc_size(2)
-void *xrealloc(void *ptr, const size_t size)
+void *xrealloc(void *ptr, const size_t nmemb, const size_t size)
 {
-        void *ret = realloc(ptr, size);
-
-        if (!ret && size)
-                err(XALLOC_EXIT_CODE, "cannot allocate %zu bytes", size);
-        return ret;
+	void *ret;
+	size_t new_size = nmemb * size;
+
+	if (new_size == 0)
+		err(XALLOC_EXIT_CODE, "xrealloc: zero size");
+	if (SIZE_MAX / nmemb < size)
+		err(XALLOC_EXIT_CODE, "xrealloc: nmemb * size > SIZE_MAX");
+	if (ptr == NULL)
+		ret = malloc(new_size);
+	else
+		ret = realloc(ptr, new_size);
+	if (!ret)
+		err(XALLOC_EXIT_CODE, "xrealloc: cannot allocate %zu bytes", size);
+	return ret;
 }
 
 static inline __ul_calloc_size(1, 2)
 void *xcalloc(const size_t nelems, const size_t size)
 {
-        void *ret = calloc(nelems, size);
-
-        if (!ret && size && nelems)
-                err(XALLOC_EXIT_CODE, "cannot allocate %zu bytes", size);
-        return ret;
+	void *ret;
+
+	if (nelems == 0 || size == 0)
+		err(XALLOC_EXIT_CODE, "xcalloc: zero size");
+	if (SIZE_MAX / nelems < size)
+		err(XALLOC_EXIT_CODE, "xcalloc: nmemb * size > SIZE_MAX");
+	ret = calloc(nelems, size);
+	if (!ret)
+		err(XALLOC_EXIT_CODE, "xcalloc: cannot allocate %zu bytes", nelems * size);
+	return ret;
 }
 
 static inline char __attribute__((warn_unused_result)) *xstrdup(const char *str)
 {
-        char *ret;
-
-        if (!str)
-                return NULL;
-
-        ret = strdup(str);
+	size_t len;
+	char *ret;
 
-        if (!ret)
-                err(XALLOC_EXIT_CODE, "cannot duplicate string");
-        return ret;
+	if (!str)
+		return NULL;
+	len = strlen(str) + 1;
+	ret = xmalloc(len);
+	memcpy(ret, str, len);
+	return ret;
 }
 
 static inline char * __attribute__((warn_unused_result)) xstrndup(const char *str, size_t size)
 {
-        char *ret;
+	char *ret;
 
-        if (!str)
-                return NULL;
+	if (!str)
+		return NULL;
 
-        ret = strndup(str, size);
+	ret = strndup(str, size);
 
-        if (!ret)
-                err(XALLOC_EXIT_CODE, "cannot duplicate string");
-        return ret;
+	if (!ret)
+		err(XALLOC_EXIT_CODE, "xstrndup: cannot duplicate string");
+	return ret;
 }
 
-
 static inline int __attribute__ ((__format__(printf, 2, 3)))
     xasprintf(char **strp, const char *fmt, ...)
 {
@@ -87,7 +103,7 @@ static inline int __attribute__ ((__format__(printf, 2, 3)))
 	ret = vasprintf(&(*strp), fmt, args);
 	va_end(args);
 	if (ret < 0)
-		err(XALLOC_EXIT_CODE, "cannot allocate string");
+		err(XALLOC_EXIT_CODE, "xasprintf: cannot allocate string");
 	return ret;
 }
 
@@ -95,11 +111,10 @@ static inline int xvasprintf(char **strp, const char *fmt, va_list ap)
 {
 	int ret = vasprintf(&(*strp), fmt, ap);
 	if (ret < 0)
-		err(XALLOC_EXIT_CODE, "cannot allocate string");
+		err(XALLOC_EXIT_CODE, "xvasprintf: cannot allocate string");
 	return ret;
 }
 
-
 static inline char * __attribute__((warn_unused_result)) xgethostname(void)
 {
 	char *name;
diff --git a/login-utils/login.c b/login-utils/login.c
index 89df489..99de89e 100644
--- a/login-utils/login.c
+++ b/login-utils/login.c
@@ -674,7 +674,7 @@ static struct passwd *get_passwd_entry(const char *username,
 			sz = (size_t) xsz;
 	}
 #endif
-	*pwdbuf = xrealloc(*pwdbuf, sz);
+	*pwdbuf = xrealloc(*pwdbuf, 1, sz);
 
 	x = getpwnam_r(username, pwd, *pwdbuf, sz, &res);
 	if (!res) {
diff --git a/misc-utils/blkid.c b/misc-utils/blkid.c
index b3eade5..46b8f24 100644
--- a/misc-utils/blkid.c
+++ b/misc-utils/blkid.c
@@ -374,7 +374,7 @@ static int append_str(char **res, size_t *sz, const char *a, const char *b)
 	if (!len)
 		return -1;
 
-	*res = str = xrealloc(str, len + 1);
+	*res = str = xrealloc(str, 1, len);
 	str += *sz;
 
 	if (a) {
diff --git a/misc-utils/findmnt.c b/misc-utils/findmnt.c
index 3386061..274db3f 100644
--- a/misc-utils/findmnt.c
+++ b/misc-utils/findmnt.c
@@ -770,7 +770,7 @@ static int parser_errcb(struct libmnt_table *tb __attribute__ ((__unused__)),
 
 static char **append_tabfile(char **files, int *nfiles, char *filename)
 {
-	files = xrealloc(files, sizeof(char *) * (*nfiles + 1));
+	files = xrealloc(files, (*nfiles + 1), sizeof(char *));
 	files[(*nfiles)++] = filename;
 	return files;
 }
diff --git a/misc-utils/getopt.c b/misc-utils/getopt.c
index 7630173..1c737e8 100644
--- a/misc-utils/getopt.c
+++ b/misc-utils/getopt.c
@@ -242,8 +242,8 @@ static void add_longopt(const char *name, int has_arg)
 	if (long_options_nr == long_options_length) {
 		long_options_length += LONG_OPTIONS_INCR;
 		long_options = xrealloc(long_options,
-					sizeof(struct option) *
-					long_options_length);
+					long_options_length,
+					sizeof(struct option));
 	}
 
 	long_options[long_options_nr].name = NULL;
diff --git a/misc-utils/logger.c b/misc-utils/logger.c
index f3bdc90..cba7412 100644
--- a/misc-utils/logger.c
+++ b/misc-utils/logger.c
@@ -231,7 +231,7 @@ static int journald_entry(FILE *fp)
 		}
 		if (lines == vectors) {
 			vectors *= 2;
-			iovec = xrealloc(iovec, vectors * sizeof(struct iovec));
+			iovec = xrealloc(iovec, vectors, sizeof(struct iovec));
 		}
 		iovec[lines].iov_base = buf;
 		iovec[lines].iov_len = sz;
diff --git a/sys-utils/swapon-common.c b/sys-utils/swapon-common.c
index 6dd7bac..7f89c47 100644
--- a/sys-utils/swapon-common.c
+++ b/sys-utils/swapon-common.c
@@ -74,7 +74,7 @@ static size_t ulct;
 
 void add_label(const char *label)
 {
-	llist = (const char **) xrealloc(llist, (++llct) * sizeof(char *));
+	llist = (const char **) xrealloc(llist, sizeof(char *), (++llct));
 	llist[llct - 1] = label;
 }
 
@@ -90,7 +90,7 @@ size_t numof_labels(void)
 
 void add_uuid(const char *uuid)
 {
-	ulist = (const char **) xrealloc(ulist, (++ulct) * sizeof(char *));
+	ulist = (const char **) xrealloc(ulist, sizeof(char *), (++ulct));
 	ulist[ulct - 1] = uuid;
 }
 
diff --git a/text-utils/col.c b/text-utils/col.c
index 9aa6a41..39b1089 100644
--- a/text-utils/col.c
+++ b/text-utils/col.c
@@ -350,8 +350,7 @@ int main(int argc, char **argv)
 			int need;
 
 			need = l->l_lsize ? l->l_lsize * 2 : 90;
-			l->l_line = (CHAR *)xrealloc((void *) l->l_line,
-						    (unsigned) need * sizeof(CHAR));
+			l->l_line = xrealloc(l->l_line, need, sizeof(CHAR));
 			l->l_lsize = need;
 		}
 		c = &l->l_line[l->l_line_len++];
@@ -460,13 +459,11 @@ void flush_line(LINE *l)
 		 */
 		if (l->l_lsize > sorted_size) {
 			sorted_size = l->l_lsize;
-			sorted = (CHAR *)xrealloc((void *)sorted,
-						  (unsigned)sizeof(CHAR) * sorted_size);
+			sorted = xrealloc(sorted, sorted_size, sizeof(CHAR));
 		}
 		if (l->l_max_col >= count_size) {
 			count_size = l->l_max_col + 1;
-			count = (int *)xrealloc((void *)count,
-			    (unsigned)sizeof(int) * count_size);
+			count = xrealloc(count, count_size, sizeof(int));
 		}
 		memset(count, 0, sizeof(int) * l->l_max_col + 1);
 		for (i = nchars, c = l->l_line; --i >= 0; c++)
diff --git a/text-utils/column.c b/text-utils/column.c
index e34184e..938c09a 100644
--- a/text-utils/column.c
+++ b/text-utils/column.c
@@ -326,8 +326,8 @@ static void maketbl(wchar_t **list, int entries, wchar_t *separator, int greedy,
 		while ((cols[coloff] = local_wcstok(p, separator, greedy, &wcstok_state)) != NULL) {
 			if (++coloff == maxcols) {
 				maxcols += DEFCOLS;
-				cols = xrealloc(cols, maxcols * sizeof(wchar_t *));
-				lens = xrealloc(lens, maxcols * sizeof(ssize_t));
+				cols = xrealloc(cols, maxcols, sizeof(wchar_t *));
+				lens = xrealloc(lens, maxcols, sizeof(ssize_t));
 				/* zero fill only new memory */
 				memset(lens + (maxcols - DEFCOLS), 0,
 				       DEFCOLS * sizeof(*lens));
@@ -398,8 +398,7 @@ static int input(FILE *fp, int *maxlength, wchar_t ***list, int *entries)
 			*maxlength = len;
 		if (local_entries == maxentry) {
 			maxentry += DEFNUM;
-			local_list = xrealloc(local_list,
-				(u_int)maxentry * sizeof(wchar_t *));
+			local_list = xrealloc(local_list, maxentry, sizeof(wchar_t *));
 		}
 		local_list[local_entries++] = wcsdup(buf);
 	}
diff --git a/text-utils/more.c b/text-utils/more.c
index d05e946..98ac72c 100644
--- a/text-utils/more.c
+++ b/text-utils/more.c
@@ -837,7 +837,7 @@ void prepare_line_buffer(void)
 		nsz = LINSIZ;
 
 	/* alloc nsz and extra space for \n\0 */
-	nline = xrealloc(Line, nsz + 2);
+	nline = xrealloc(Line, 2, nsz);
 	Line = nline;
 	LineLen = nsz;
 }
@@ -2059,7 +2059,7 @@ int expand(char **outbuf, char *inbuf)
 		offset = outstr - temp;
 		if (tempsz - offset - 1 < xtra) {
 			tempsz += 200 + xtra;
-			temp = xrealloc(temp, tempsz);
+			temp = xrealloc(temp, 1, tempsz);
 			outstr = temp + offset;
 		}
 		switch (c) {
diff --git a/text-utils/rev.c b/text-utils/rev.c
index f1341cb..ed4c313 100644
--- a/text-utils/rev.c
+++ b/text-utils/rev.c
@@ -155,7 +155,7 @@ int main(int argc, char *argv[])
 				/* So now we double the buffer size */
 				bufsiz *= 2;
 
-				buf = xrealloc(buf, bufsiz * sizeof(wchar_t));
+				buf = xrealloc(buf, bufsiz, sizeof(wchar_t));
 
 				/* And fill the rest of the buffer */
 				if (!fgetws(&buf[len], bufsiz/2, fp))
diff --git a/text-utils/ul.c b/text-utils/ul.c
index b3b3dc3..ead1d52 100644
--- a/text-utils/ul.c
+++ b/text-utils/ul.c
@@ -653,7 +653,7 @@ static void needcol(int acol) {
 			obuflen = INT_MAX;
 
 		/* Now we can try to expand obuf. */
-		obuf = xrealloc(obuf, sizeof(struct CHAR) * obuflen);
+		obuf = xrealloc(obuf, obuflen, sizeof(struct CHAR));
 	}
 }
 
-- 
1.9.2

Re: [PATCH 01/12] include/xalloc: ensure arithmetics overflow cannot happen

[Bernhard Voelker]

On 04/27/2014 10:05 PM, Sami Kerola wrote:
> The xrealloc() changes has the greatest change.  It splits the size and
> multiplier arguments so that arithmetics overflow can be detected.  This
> change is propagated to use of the function in other files.
> 
> Additionally this change checks that size inputs for allocations are
> never zero.  It is uncertain if in these cases abort() should be called
> to get a core.

I'd favor to see the behavior of the allocation functions to be harmonized
with gnulib: quite a couple of us guys may work in projects using it, thus
being familiar with its details and corner cases.
WDYT?

> The xstrdup() is made to use memcpy(), which is exactly what the library
> call does so one layer of absraction is saved here.
...
>  static inline char __attribute__((warn_unused_result)) *xstrdup(const char *str)
>  {
> -        char *ret;
> -
> -        if (!str)
> -                return NULL;
> -
> -        ret = strdup(str);
> +	size_t len;
> +	char *ret;
>  
> -        if (!ret)
> -                err(XALLOC_EXIT_CODE, "cannot duplicate string");
> -        return ret;
> +	if (!str)
> +		return NULL;
> +	len = strlen(str) + 1;
> +	ret = xmalloc(len);
> +	memcpy(ret, str, len);
> +	return ret;
>  }

Hmm, while memcpy() alone is faster than strcpy(), replacing the
latter by strlen() + memcpy() certainly is not.  The compilers and
libc are optimized enough, e.g. by using had-crafted assembler code,
that I think you don't have a chance to be faster by trying to be
smarter than them.

Have a nice day,
Berny

Re: [PATCH 01/12] include/xalloc: ensure arithmetics overflow cannot happen

[Karel Zak]

On Sun, Apr 27, 2014 at 09:05:27PM +0100, Sami Kerola wrote:
> The xrealloc() changes has the greatest change.  It splits the size and
> multiplier arguments so that arithmetics overflow can be detected.  This
> change is propagated to use of the function in other files.

 I don't like it at all. The function realloc() has well know semantic
 and arguments. We don't want to create parallel universe...
 
 If you want something else "nmemb, size"  then introduce xrecalloc()
 or so.. but don't use "realloc" name at all.

> Additionally this change checks that size inputs for allocations are
> never zero.  It is uncertain if in these cases abort() should be called
> to get a core.

 I don't think we need a different semantic than C standards.

>  void *xmalloc(const size_t size)
>  {
> -        void *ret = malloc(size);
> +	void *ret;
>  
> -        if (!ret && size)
> -                err(XALLOC_EXIT_CODE, "cannot allocate %zu bytes", size);
> -        return ret;
> +	if (size == 0)
> +		err(XALLOC_EXIT_CODE, "xmalloc: zero size");

 man malloc, zero size is just correct

> +	ret = malloc(size);
> +	if (!ret)
> +		err(XALLOC_EXIT_CODE, "xmalloc: cannot allocate %zu bytes", size);
> +	return ret;
>  }

 I don't think we need "xmalloc:" prefix to the error message.

>  static inline __ul_alloc_size(2)
> -void *xrealloc(void *ptr, const size_t size)
> +void *xrealloc(void *ptr, const size_t nmemb, const size_t size)
>  {
> -        void *ret = realloc(ptr, size);
> -
> -        if (!ret && size)
> -                err(XALLOC_EXIT_CODE, "cannot allocate %zu bytes", size);
> -        return ret;
> +	void *ret;
> +	size_t new_size = nmemb * size;
> +
> +	if (new_size == 0)
> +		err(XALLOC_EXIT_CODE, "xrealloc: zero size");

 man realloc, zero size is correct

> +	if (SIZE_MAX / nmemb < size)
> +		err(XALLOC_EXIT_CODE, "xrealloc: nmemb * size > SIZE_MAX");
> +	if (ptr == NULL)
> +		ret = malloc(new_size);
> +	else
> +		ret = realloc(ptr, new_size);
> +	if (!ret)
> +		err(XALLOC_EXIT_CODE, "xrealloc: cannot allocate %zu bytes", size);
> +	return ret;
>  }
>  
>  static inline __ul_calloc_size(1, 2)
>  void *xcalloc(const size_t nelems, const size_t size)
>  {
> -        void *ret = calloc(nelems, size);
> -
> -        if (!ret && size && nelems)
> -                err(XALLOC_EXIT_CODE, "cannot allocate %zu bytes", size);
> -        return ret;
> +	void *ret;
> +
> +	if (nelems == 0 || size == 0)
> +		err(XALLOC_EXIT_CODE, "xcalloc: zero size");

 zero size is correct

> +	if (SIZE_MAX / nelems < size)
> +		err(XALLOC_EXIT_CODE, "xcalloc: nmemb * size > SIZE_MAX");
> +	ret = calloc(nelems, size);
> +	if (!ret)
> +		err(XALLOC_EXIT_CODE, "xcalloc: cannot allocate %zu bytes", nelems * size);
> +	return ret;
>  }
>  
>  static inline char __attribute__((warn_unused_result)) *xstrdup(const char *str)
>  {
> -        char *ret;
> -
> -        if (!str)
> -                return NULL;
> -
> -        ret = strdup(str);
> +	size_t len;
> +	char *ret;
>  
> -        if (!ret)
> -                err(XALLOC_EXIT_CODE, "cannot duplicate string");
> -        return ret;
> +	if (!str)
> +		return NULL;
> +	len = strlen(str) + 1;
> +	ret = xmalloc(len);
> +	memcpy(ret, str, len);
> +	return ret;
>  }

 Seem like premature optimization, it would be better to use libc
 rather than maintain private implementation.

    Karel


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

Re: [PATCH 01/12] include/xalloc: ensure arithmetics overflow cannot happen

[Sami Kerola]

On 28 April 2014 07:52, Karel Zak <kzak@redhat.com> wrote:
> On Sun, Apr 27, 2014 at 09:05:27PM +0100, Sami Kerola wrote:
>> The xrealloc() changes has the greatest change.  It splits the size and
>> multiplier arguments so that arithmetics overflow can be detected.  This
>> change is propagated to use of the function in other files.
>
>  I don't like it at all. The function realloc() has well know semantic
>  and arguments. We don't want to create parallel universe...
>
>  If you want something else "nmemb, size"  then introduce xrecalloc()
>  or so.. but don't use "realloc" name at all.
>
>> Additionally this change checks that size inputs for allocations are
>> never zero.  It is uncertain if in these cases abort() should be called
>> to get a core.
>
>  I don't think we need a different semantic than C standards.

Hi Karel,

Considering that, and other feedback, this patch is completely dead.
Please skip, and I'll have look if there is some value to add
xrecalloc() to xalloc.h.

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

[PATCH 02/12] dmesg: move get_boot_time() to lib/timeutils

[Sami Kerola]

In future the last(1) will use get_boot_time() as well.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/timeutils.h |  1 +
 lib/timeutils.c     | 29 +++++++++++++++++++++++++++++
 sys-utils/dmesg.c   | 27 ---------------------------
 3 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/include/timeutils.h b/include/timeutils.h
index bcae613..b9ba0f7 100644
--- a/include/timeutils.h
+++ b/include/timeutils.h
@@ -51,5 +51,6 @@ typedef uint64_t nsec_t;
 #define FORMAT_TIMESPAN_MAX 64
 
 int parse_timestamp(const char *t, usec_t *usec);
+int get_boot_time(struct timeval *boot_time);
 
 #endif /* UTIL_LINUX_TIME_UTIL_H */
diff --git a/lib/timeutils.c b/lib/timeutils.c
index 7fe6218..c1b632e 100644
--- a/lib/timeutils.c
+++ b/lib/timeutils.c
@@ -21,9 +21,12 @@
 #include <assert.h>
 #include <ctype.h>
 #include <string.h>
+#include <sys/sysinfo.h>
+#include <sys/time.h>
 #include <time.h>
 
 #include "c.h"
+#include "nls.h"
 #include "strutils.h"
 #include "timeutils.h"
 
@@ -336,3 +339,29 @@ int parse_timestamp(const char *t, usec_t *usec)
 
 	return 0;
 }
+
+int get_boot_time(struct timeval *boot_time)
+{
+	struct timespec hires_uptime;
+	struct timeval lores_uptime, now;
+	struct sysinfo info;
+
+	if (gettimeofday(&now, NULL) != 0) {
+		warn(_("gettimeofday failed"));
+		return -errno;
+	}
+#ifdef CLOCK_BOOTTIME
+	if (clock_gettime(CLOCK_BOOTTIME, &hires_uptime) == 0) {
+		TIMESPEC_TO_TIMEVAL(&lores_uptime, &hires_uptime);
+		timersub(&now, &lores_uptime, boot_time);
+		return 0;
+	}
+#endif
+	/* fallback */
+	if (sysinfo(&info) != 0)
+		warn(_("sysinfo failed"));
+
+	boot_time->tv_sec = now.tv_sec - info.uptime;
+	boot_time->tv_usec = 0;
+	return 0;
+}
diff --git a/sys-utils/dmesg.c b/sys-utils/dmesg.c
index df68c84..196cc61 100644
--- a/sys-utils/dmesg.c
+++ b/sys-utils/dmesg.c
@@ -472,33 +472,6 @@ static int get_syslog_buffer_size(void)
 	return n > 0 ? n : 0;
 }
 
-static int get_boot_time(struct timeval *boot_time)
-{
-	struct timespec hires_uptime;
-	struct timeval lores_uptime, now;
-	struct sysinfo info;
-
-	if (gettimeofday(&now, NULL) != 0) {
-		warn(_("gettimeofday failed"));
-		return -errno;
-	}
-
-#ifdef CLOCK_BOOTTIME
-	if (clock_gettime(CLOCK_BOOTTIME, &hires_uptime) == 0) {
-		TIMESPEC_TO_TIMEVAL(&lores_uptime, &hires_uptime);
-		timersub(&now, &lores_uptime, boot_time);
-		return 0;
-	}
-#endif
-	/* fallback */
-	if (sysinfo(&info) != 0)
-		warn(_("sysinfo failed"));
-
-	boot_time->tv_sec = now.tv_sec - info.uptime;
-	boot_time->tv_usec = 0;
-	return 0;
-}
-
 /*
  * Reads messages from regular file by mmap
  */
-- 
1.9.2

Re: [PATCH 02/12] dmesg: move get_boot_time() to lib/timeutils

[Ruediger Meier]

On Sunday 27 April 2014, Sami Kerola wrote:
> In future the last(1) will use get_boot_time() as well.
>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  include/timeutils.h |  1 +
>  lib/timeutils.c     | 29 +++++++++++++++++++++++++++++
>  sys-utils/dmesg.c   | 27 ---------------------------
>  3 files changed, 30 insertions(+), 27 deletions(-)
>
> [...]
> -
> -#ifdef CLOCK_BOOTTIME
> -	if (clock_gettime(CLOCK_BOOTTIME, &hires_uptime) == 0) {
> -		TIMESPEC_TO_TIMEVAL(&lores_uptime, &hires_uptime);
> -		timersub(&now, &lores_uptime, boot_time);
> -		return 0;
> -	}

clock_gettime() needs -lrt on some systems. That's why we had 
dmesg_LDADD += -lrt. Now every binary which links against libcommon 
needs that.

cu,
Rudi

Re: [PATCH 02/12] dmesg: move get_boot_time() to lib/timeutils

[Karel Zak]

On Tue, May 06, 2014 at 12:00:00PM +0200, Ruediger Meier wrote:
> On Sunday 27 April 2014, Sami Kerola wrote:
> > In future the last(1) will use get_boot_time() as well.
> >
> > Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> > ---
> >  include/timeutils.h |  1 +
> >  lib/timeutils.c     | 29 +++++++++++++++++++++++++++++
> >  sys-utils/dmesg.c   | 27 ---------------------------
> >  3 files changed, 30 insertions(+), 27 deletions(-)
> >
> > [...]
> > -
> > -#ifdef CLOCK_BOOTTIME
> > -	if (clock_gettime(CLOCK_BOOTTIME, &hires_uptime) == 0) {
> > -		TIMESPEC_TO_TIMEVAL(&lores_uptime, &hires_uptime);
> > -		timersub(&now, &lores_uptime, boot_time);
> > -		return 0;
> > -	}
> 
> clock_gettime() needs -lrt on some systems. That's why we had 
> dmesg_LDADD += -lrt. Now every binary which links against libcommon 
> needs that.

 Fixed, I have moved get_boot_time() outside libcommon. The question
 is how portable will be last(1), maybe it will necessary to add
 something like "if HAVE_LIBRT" to the build system, now there is 
 hardcoded -lrt.

    Karel

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

Re: [PATCH 02/12] dmesg: move get_boot_time() to lib/timeutils

[Ruediger Meier]

On Tuesday 06 May 2014, Karel Zak wrote:
> On Tue, May 06, 2014 at 12:00:00PM +0200, Ruediger Meier wrote:

> > clock_gettime() needs -lrt on some systems. That's why we had
> > dmesg_LDADD += -lrt. Now every binary which links against libcommon
> > needs that.
>
>  Fixed, I have moved get_boot_time() outside libcommon.

Thanks, you still have forgotten to commit the new header
  include/boottime.h

cu,
Rudi

Re: [PATCH 02/12] dmesg: move get_boot_time() to lib/timeutils

[Karel Zak]

On Tue, May 06, 2014 at 03:42:44PM +0200, Ruediger Meier wrote:
> On Tuesday 06 May 2014, Karel Zak wrote:
> > On Tue, May 06, 2014 at 12:00:00PM +0200, Ruediger Meier wrote:
> 
> > > clock_gettime() needs -lrt on some systems. That's why we had
> > > dmesg_LDADD += -lrt. Now every binary which links against libcommon
> > > needs that.
> >
> >  Fixed, I have moved get_boot_time() outside libcommon.
> 
> Thanks, you still have forgotten to commit the new header
>   include/boottime.h

 Ooooh, fixed...
 
 It would be nice to have any connection between build system and git
 to avoid just stupid (but common) mistakes.

    Karel

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

Re: [PATCH 02/12] dmesg: move get_boot_time() to lib/timeutils

[Ruediger Meier]

On Tuesday 06 May 2014, Karel Zak wrote:
> On Tue, May 06, 2014 at 03:42:44PM +0200, Ruediger Meier wrote:
> > On Tuesday 06 May 2014, Karel Zak wrote:
> > > On Tue, May 06, 2014 at 12:00:00PM +0200, Ruediger Meier wrote:
> > > > clock_gettime() needs -lrt on some systems. That's why we had
> > > > dmesg_LDADD += -lrt. Now every binary which links against
> > > > libcommon needs that.
> > >
> > >  Fixed, I have moved get_boot_time() outside libcommon.
> >
> > Thanks, you still have forgotten to commit the new header
> >   include/boottime.h
>
>  Ooooh, fixed...
>
>  It would be nice to have any connection between build system and git
>  to avoid just stupid (but common) mistakes.

We have "make distcheck" and because it's annoying to do this manually 
before any push there is "travis" to get it done automatically at least 
a few minutes after.

I had ask you once if you could sign up and enable travis, see
https://github.com/karelzak/util-linux/pull/65
Would be really nice to have it.

BTW bootime.c is still missing ;)

cu,
Rudi

Re: [PATCH 02/12] dmesg: move get_boot_time() to lib/timeutils

[Karel Zak]

On Tue, May 06, 2014 at 06:03:46PM +0200, Ruediger Meier wrote:
> I had ask you once if you could sign up and enable travis, see
> https://github.com/karelzak/util-linux/pull/65
> Would be really nice to have it.

 Done, it seems it works. 
 
 (but I hope you will still contribute to u-l  ;-)

    Karel


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

Re: [PATCH 02/12] dmesg: move get_boot_time() to lib/timeutils

[Ruediger Meier]

On Wednesday 07 May 2014, Karel Zak wrote:
> On Tue, May 06, 2014 at 06:03:46PM +0200, Ruediger Meier wrote:
> > I had ask you once if you could sign up and enable travis, see
> > https://github.com/karelzak/util-linux/pull/65
> > Would be really nice to have it.
>
>  Done, it seems it works.

Thanks!

>  (but I hope you will still contribute to u-l  ;-)

Hehe, that's a good point :)


cu,
Rudi

[PATCH 03/12] last: fix is_phantom() detection

[Sami Kerola]

The /proc/<pid>/loginuid is not always available, and when so a running
session should not be determined to be gone.  This is a regression from
commit mentioned in reference.

Sessions that have started before previous system boot, and did not log
out meanwhile, will be marked as gone.  It is fair to say that these
sessions are most likely result of a wtmp corruption.

Reference: 404fa3f93c00c7e130f5a0ec963b2dc6a3743986
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/last.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/login-utils/last.c b/login-utils/last.c
index 5122f8e..ed10586 100644
--- a/login-utils/last.c
+++ b/login-utils/last.c
@@ -94,6 +94,7 @@ struct last_control {
 	unsigned int altc;	/* Number of alternative files */
 	unsigned int alti;	/* Index number of the alternative file */
 
+	struct timeval boot_time; /* system boot time */
 	time_t since;		/* at what time to start displaying the file */
 	time_t until;		/* at what time to stop displaying the file */
 	time_t present;		/* who where present at time_t */
@@ -573,19 +574,24 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
 	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
 }
 
-static int is_phantom(struct utmp *ut)
+static int is_phantom(const struct last_control *ctl, struct utmp *ut)
 {
 	struct passwd *pw;
 	char path[32];
 	FILE *f;
-	unsigned int loginuid, ret = 0;
+	unsigned int loginuid;
+	int ret = 0;
 
+	if (ut->UL_UT_TIME < ctl->boot_time.tv_sec)
+		return 1;
 	pw = getpwnam(ut->ut_name);
 	if (!pw)
 		return 1;
 	sprintf(path, "/proc/%u/loginuid", ut->ut_pid);
-	if (!(f = fopen(path, "r")))
+	if (access(path, R_OK) == 0 && !(f = fopen(path, "r")))
 		return 1;
+	else
+		return ret;
 	if (fscanf(f, "%u", &loginuid) != 1)
 		ret = 1;
 	fclose(f);
@@ -787,7 +793,7 @@ static void process_wtmp_file(const struct last_control *ctl)
 				if (!lastboot) {
 					c = R_NOW;
 					/* Is process still alive? */
-					if (is_phantom(&ut))
+					if (is_phantom(ctl, &ut))
 						c = R_PHANTOM;
 				} else
 					c = whydown;
@@ -980,6 +986,7 @@ int main(int argc, char **argv)
 	}
 
 	for (; ctl.alti < ctl.altc; ctl.alti++) {
+		get_boot_time(&ctl.boot_time);
 		process_wtmp_file(&ctl);
 		free(ctl.altv[ctl.alti]);
 	}
-- 
1.9.2

[PATCH 04/12] include/c.h: add macro to print definitions as string

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/c.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/c.h b/include/c.h
index a192fb1..6b8793a 100644
--- a/include/c.h
+++ b/include/c.h
@@ -307,4 +307,13 @@ static inline int xusleep(useconds_t usec)
 # define SEEK_HOLE	4
 #endif
 
+
+/*
+ * Macros to convert #define'itions to strings, for example
+ * #define XYXXY 42
+ * printf ("%s=%s\n", stringify(XYXXY), stringify_value(XYXXY));
+ */
+#define stringify_value(s) stringify(s)
+#define stringify(s) #s
+
 #endif /* UTIL_LINUX_C_H */
-- 
1.9.2

[PATCH 05/12] mkswap, swaplabel: move version number to header

[Sami Kerola]

Corrently only the swap version 1 is supported, which is a magic value so
move it to header.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/mkswap.c    | 10 +++++-----
 disk-utils/swaplabel.c |  4 ++--
 include/swapheader.h   |  1 +
 sys-utils/swapon.c     | 10 ++++------
 4 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/disk-utils/mkswap.c b/disk-utils/mkswap.c
index ae73f80..cd96ad5 100644
--- a/disk-utils/mkswap.c
+++ b/disk-utils/mkswap.c
@@ -449,7 +449,7 @@ main(int argc, char **argv) {
 	unsigned long long sz;
 	off_t offset;
 	int force = 0;
-	int version = 1;
+	int version = SWAP_VERSION;
 	char *block_count = 0;
 	char *opt_label = NULL;
 	unsigned char *uuid = NULL;
@@ -517,7 +517,7 @@ main(int argc, char **argv) {
 		usage(stderr);
 	}
 
-	if (version != 1)
+	if (version != SWAP_VERSION)
 		errx(EXIT_FAILURE,
 			_("swapspace version %d is not supported"), version);
 
@@ -601,7 +601,7 @@ main(int argc, char **argv) {
 	wipe_device(DEV, device_name, force);
 
 	hdr = (struct swap_header_v1_2 *) signature_page;
-	hdr->version = 1;
+	hdr->version = version;
 	hdr->last_page = PAGES - 1;
 	hdr->nr_badpages = badpages;
 
@@ -609,8 +609,8 @@ main(int argc, char **argv) {
 		errx(EXIT_FAILURE, _("Unable to set up swap-space: unreadable"));
 
 	goodpages = PAGES - badpages - 1;
-	printf(_("Setting up swapspace version 1, size = %llu KiB\n"),
-		goodpages * pagesize / 1024);
+	printf(_("Setting up swapspace version %d, size = %llu KiB\n"),
+		version, goodpages * pagesize / 1024);
 
 	write_signature("SWAPSPACE2");
 	write_uuid_and_label(uuid, opt_label);
diff --git a/disk-utils/swaplabel.c b/disk-utils/swaplabel.c
index 5d048aa..8d5b260 100644
--- a/disk-utils/swaplabel.c
+++ b/disk-utils/swaplabel.c
@@ -67,10 +67,10 @@ static blkid_probe get_swap_prober(const char *devname)
 		warnx(_("%s: not a valid swap partition"), devname);
 
 	if (rc == 0) {
-		/* supported is SWAPSPACE2 only */
+		/* Only the SWAPSPACE2 is supported. */
 		if (blkid_probe_lookup_value(pr, "VERSION", &version, NULL) == 0
 		    && version
-		    && strcmp(version, "1"))
+		    && strcmp(version, stringify_value(SWAP_VERSION)))
 			warnx(_("%s: unsupported swap version '%s'"),
 						devname, version);
 		else
diff --git a/include/swapheader.h b/include/swapheader.h
index 42d521a..80fa36b 100644
--- a/include/swapheader.h
+++ b/include/swapheader.h
@@ -11,6 +11,7 @@ struct swap_header_v1 {
 };
 
 
+#define SWAP_VERSION 1
 #define SWAP_UUID_LENGTH 16
 #define SWAP_LABEL_LENGTH 16
 
diff --git a/sys-utils/swapon.c b/sys-utils/swapon.c
index 3866ee7..6c422ce 100644
--- a/sys-utils/swapon.c
+++ b/sys-utils/swapon.c
@@ -402,21 +402,19 @@ static unsigned long long swap_get_size(const char *hdr, const char *devname,
 					unsigned int pagesize)
 {
 	unsigned int last_page = 0;
-	int swap_version = 0;
+	const unsigned int swap_version = SWAP_VERSION;
 	int flip = 0;
 	struct swap_header_v1_2 *s;
 
 	s = (struct swap_header_v1_2 *) hdr;
-	if (s->version == 1) {
-		swap_version = 1;
+	if (s->version == swap_version) {
 		last_page = s->last_page;
-	} else if (swab32(s->version) == 1) {
+	} else if (swab32(s->version) == swap_version) {
 		flip = 1;
-		swap_version = 1;
 		last_page = swab32(s->last_page);
 	}
 	if (verbose)
-		warnx(_("%s: found swap signature: version %d, "
+		warnx(_("%s: found swap signature: version %ud, "
 			"page-size %d, %s byte order"),
 			devname,
 			swap_version,
-- 
1.9.2

[PATCH 06/12] mkswap: remove legacy swap structure

[Sami Kerola]

If software archeolgists want to know how the old, and unused, swap
header looked they can dig it from the revision history.

Reference: 4c85aa3a4c26f2a2c33bf16960b548d5bbd5b4bf
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/mkswap.c  |  3 +--
 include/swapheader.h | 11 +----------
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/disk-utils/mkswap.c b/disk-utils/mkswap.c
index cd96ad5..830b14c 100644
--- a/disk-utils/mkswap.c
+++ b/disk-utils/mkswap.c
@@ -186,8 +186,7 @@ write_uuid_and_label(unsigned char *uuid, char *volume_name)
 	struct swap_header_v1_2 *h;
 
 	/* Sanity check */
-	if (sizeof(struct swap_header_v1) !=
-	    sizeof(struct swap_header_v1_2)) {
+	if (sizeof(struct swap_header_v1_2) != SWAP_HEADER_SIZE) {
 		warnx(_("Bad swap header size, no label written."));
 		return;
 	}
diff --git a/include/swapheader.h b/include/swapheader.h
index 80fa36b..1fdd133 100644
--- a/include/swapheader.h
+++ b/include/swapheader.h
@@ -1,19 +1,10 @@
 #ifndef _SWAPHEADER_H
 #define _SWAPHEADER_H
 
-struct swap_header_v1 {
-        char         bootbits[1024];    /* Space for disklabel etc. */
-	unsigned int version;
-	unsigned int last_page;
-	unsigned int nr_badpages;
-	unsigned int padding[125];
-	unsigned int badpages[1];
-};
-
-
 #define SWAP_VERSION 1
 #define SWAP_UUID_LENGTH 16
 #define SWAP_LABEL_LENGTH 16
+#define SWAP_HEADER_SIZE 1540
 
 struct swap_header_v1_2 {
 	char	      bootbits[1024];    /* Space for disklabel etc. */
-- 
1.9.2

[PATCH 07/12] include/swapheader.h: ensure type sizes

[Sami Kerola]

Use consistently the same type sizes as in libblkid and kernel.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/swapheader.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/swapheader.h b/include/swapheader.h
index 1fdd133..1e1090c 100644
--- a/include/swapheader.h
+++ b/include/swapheader.h
@@ -6,15 +6,17 @@
 #define SWAP_LABEL_LENGTH 16
 #define SWAP_HEADER_SIZE 1540
 
+#include <stdint.h>
+
 struct swap_header_v1_2 {
 	char	      bootbits[1024];    /* Space for disklabel etc. */
-	unsigned int  version;
-	unsigned int  last_page;
-	unsigned int  nr_badpages;
+	uint32_t      version;
+	uint32_t      last_page;
+	uint32_t      nr_badpages;
 	unsigned char uuid[SWAP_UUID_LENGTH];
 	char	      volume_name[SWAP_LABEL_LENGTH];
-	unsigned int  padding[117];
-	unsigned int  badpages[1];
+	uint32_t      padding[117];
+	uint32_t      badpages[1];
 };
 
 #endif /* _SWAPHEADER_H */
-- 
1.9.2

[PATCH 08/12] swapon: swaps with legacy version label are not supported

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/swapon.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sys-utils/swapon.c b/sys-utils/swapon.c
index 6c422ce..c2d4795 100644
--- a/sys-utils/swapon.c
+++ b/sys-utils/swapon.c
@@ -344,8 +344,7 @@ err:
 
 static int swap_detect_signature(const char *buf, int *sig)
 {
-	if (memcmp(buf, "SWAP-SPACE", 10) == 0 ||
-            memcmp(buf, "SWAPSPACE2", 10) == 0)
+	if (memcmp(buf, SWAP_SIGNATURE, 10) == 0)
 		*sig = SIG_SWAPSPACE;
 
 	else if (memcmp(buf, "S1SUSPEND", 9) == 0 ||
-- 
1.9.2

Re: [PATCH 08/12] swapon: swaps with legacy version label are not supported

[Benno Schulenberg]

On Sun, Apr 27, 2014, at 22:05, Sami Kerola wrote:
> -	if (memcmp(buf, "SWAP-SPACE", 10) == 0 ||
> -            memcmp(buf, "SWAPSPACE2", 10) == 0)
> +	if (memcmp(buf, SWAP_SIGNATURE, 10) == 0)

s/10/SWAP_SIGNATURE_SZ/  ?

Benno

-- 
http://www.fastmail.fm - Accessible with your email software
                          or over the web

Re: [PATCH 08/12] swapon: swaps with legacy version label are not supported

[Sami Kerola]

On 28 April 2014 09:37, Benno Schulenberg <bensberg@justemail.net> wrote:
>
> On Sun, Apr 27, 2014, at 22:05, Sami Kerola wrote:
>> -     if (memcmp(buf, "SWAP-SPACE", 10) == 0 ||
>> -            memcmp(buf, "SWAPSPACE2", 10) == 0)
>> +     if (memcmp(buf, SWAP_SIGNATURE, 10) == 0)
>
> s/10/SWAP_SIGNATURE_SZ/  ?

Hi Benno,

Yes, that would be better. Karel, can you amend & add Benno as reviewer?

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

Re: [PATCH 08/12] swapon: swaps with legacy version label are not supported

[Karel Zak]

On Mon, Apr 28, 2014 at 09:44:01AM +0100, Sami Kerola wrote:
> On 28 April 2014 09:37, Benno Schulenberg <bensberg@justemail.net> wrote:
> >
> > On Sun, Apr 27, 2014, at 22:05, Sami Kerola wrote:
> >> -     if (memcmp(buf, "SWAP-SPACE", 10) == 0 ||
> >> -            memcmp(buf, "SWAPSPACE2", 10) == 0)
> >> +     if (memcmp(buf, SWAP_SIGNATURE, 10) == 0)
> >
> > s/10/SWAP_SIGNATURE_SZ/  ?
> 
> Hi Benno,
> 
> Yes, that would be better. Karel, can you amend & add Benno as reviewer?

 Sure, what about to add macro is_swap_magic() to include/ to hide all the
 details? BTW, there is also libblkid/src/superblocks/swap.c :-)

    Karel

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

[PATCH 09/12] swapon, swapheader, mkswap: move swap signature to header

[Sami Kerola]

Both swapon and mkswap need to know what is valid device signature, so
share the value.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/mkswap.c  | 4 ++--
 include/swapheader.h | 2 ++
 sys-utils/swapon.c   | 3 ---
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/disk-utils/mkswap.c b/disk-utils/mkswap.c
index 830b14c..16b8e89 100644
--- a/disk-utils/mkswap.c
+++ b/disk-utils/mkswap.c
@@ -177,7 +177,7 @@ write_signature(char *sig)
 {
 	char *sp = (char *) signature_page;
 
-	strncpy(sp + pagesize - 10, sig, 10);
+	strncpy(sp + pagesize - 10, sig, SWAP_SIGNATURE_SZ);
 }
 
 static void
@@ -611,7 +611,7 @@ main(int argc, char **argv) {
 	printf(_("Setting up swapspace version %d, size = %llu KiB\n"),
 		version, goodpages * pagesize / 1024);
 
-	write_signature("SWAPSPACE2");
+	write_signature(SWAP_SIGNATURE);
 	write_uuid_and_label(uuid, opt_label);
 
 	offset = 1024;
diff --git a/include/swapheader.h b/include/swapheader.h
index 1e1090c..9ca9673 100644
--- a/include/swapheader.h
+++ b/include/swapheader.h
@@ -5,6 +5,8 @@
 #define SWAP_UUID_LENGTH 16
 #define SWAP_LABEL_LENGTH 16
 #define SWAP_HEADER_SIZE 1540
+#define SWAP_SIGNATURE "SWAPSPACE2"
+#define SWAP_SIGNATURE_SZ (sizeof(SWAP_SIGNATURE) - 1)
 
 #include <stdint.h>
 
diff --git a/sys-utils/swapon.c b/sys-utils/swapon.c
index c2d4795..f321b05 100644
--- a/sys-utils/swapon.c
+++ b/sys-utils/swapon.c
@@ -76,9 +76,6 @@ enum {
 	SIG_SWSUSPEND
 };
 
-#define SWAP_SIGNATURE		"SWAPSPACE2"
-#define SWAP_SIGNATURE_SZ	(sizeof(SWAP_SIGNATURE) - 1)
-
 static int all;
 static int priority = -1;	/* non-prioritized swap by default */
 static int discard;		/* don't send swap discards by default */
-- 
1.9.2

[PATCH 10/12] libsmartcols: remove ununsed assignment

[Sami Kerola]

Value stored to 'rc' is never read.

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

diff --git a/libsmartcols/src/table_print.c b/libsmartcols/src/table_print.c
index 2942053..4df587e 100644
--- a/libsmartcols/src/table_print.c
+++ b/libsmartcols/src/table_print.c
@@ -739,7 +739,7 @@ int scols_print_table(struct libscols_table *tb)
 		return -ENOMEM;
 
 	if (!(scols_table_is_raw(tb) || scols_table_is_export(tb)))
-		rc = recount_widths(tb, buf);
+		recount_widths(tb, buf);
 
 	if (scols_table_is_tree(tb))
 		rc = print_tree(tb, buf);
-- 
1.9.2

Re: [PATCH 10/12] libsmartcols: remove ununsed assignment

[Karel Zak]

On Sun, Apr 27, 2014 at 09:05:36PM +0100, Sami Kerola wrote:
> Value stored to 'rc' is never read.
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  libsmartcols/src/table_print.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libsmartcols/src/table_print.c b/libsmartcols/src/table_print.c
> index 2942053..4df587e 100644
> --- a/libsmartcols/src/table_print.c
> +++ b/libsmartcols/src/table_print.c
> @@ -739,7 +739,7 @@ int scols_print_table(struct libscols_table *tb)
>  		return -ENOMEM;
>  
>  	if (!(scols_table_is_raw(tb) || scols_table_is_export(tb)))
> -		rc = recount_widths(tb, buf);
> +		recount_widths(tb, buf);

 It would be better to check 'rc'.

        if (rc)
           goto done;

 or so...

    Karel

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

[PATCH 11/12] lib/timeutils: fix memory leak

[Sami Kerola]

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

diff --git a/lib/timeutils.c b/lib/timeutils.c
index c1b632e..d542c2f 100644
--- a/lib/timeutils.c
+++ b/lib/timeutils.c
@@ -239,6 +239,7 @@ int parse_timestamp(const char *t, usec_t *usec)
 			return -ENOMEM;
 
 		r = parse_sec(z, &minus);
+		free(z);
 		if (r < 0)
 			return r;
 
-- 
1.9.2

[PATCH 12/12] lib/pager: use names when referring to standard file descriptors

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 lib/pager.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/lib/pager.c b/lib/pager.c
index c6e74e8..9e09cd5 100644
--- a/lib/pager.c
+++ b/lib/pager.c
@@ -63,10 +63,10 @@ static int start_command(struct child_process *cmd)
 	cmd->pid = fork();
 	if (!cmd->pid) {
 		if (need_in) {
-			dup2(fdin[0], 0);
+			dup2(fdin[0], STDIN_FILENO);
 			close_pair(fdin);
 		} else if (cmd->in > 0) {
-			dup2(cmd->in, 0);
+			dup2(cmd->in, STDIN_FILENO);
 			close(cmd->in);
 		}
 
@@ -134,7 +134,7 @@ static void pager_preexec(void)
 	fd_set in;
 
 	FD_ZERO(&in);
-	FD_SET(0, &in);
+	FD_SET(STDIN_FILENO, &in);
 	select(1, &in, NULL, &in, NULL);
 
 	setenv("LESS", "FRSX", 0);
@@ -145,8 +145,8 @@ static void wait_for_pager(void)
 	fflush(stdout);
 	fflush(stderr);
 	/* signal EOF to pager */
-	close(1);
-	close(2);
+	close(STDOUT_FILENO);
+	close(STDERR_FILENO);
 	finish_command(&pager_process);
 }
 
@@ -160,7 +160,7 @@ void setup_pager(void)
 {
 	const char *pager = getenv("PAGER");
 
-	if (!isatty(1))
+	if (!isatty(STDOUT_FILENO))
 		return;
 
 	if (!pager)
@@ -178,9 +178,9 @@ void setup_pager(void)
 		return;
 
 	/* original process continues, but writes to the pipe */
-	dup2(pager_process.in, 1);
-	if (isatty(2))
-		dup2(pager_process.in, 2);
+	dup2(pager_process.in, STDOUT_FILENO);
+	if (isatty(STDERR_FILENO))
+		dup2(pager_process.in, STDERR_FILENO);
 	close(pager_process.in);
 
 	/* this makes sure that the parent terminates after the pager */
-- 
1.9.2

Re: [PATCH 00/12] pull: mostly swap command clarifications

[Sami Kerola]

On 27 April 2014 21:05, Sami Kerola <kerolasa@iki.fi> wrote:
> are available in the git repository at:
>
>   git://github.com/kerolasa/lelux-utiliteetit.git 2014wk16

[snip]

>   include/xalloc: ensure arithmetics overflow cannot happen
>   dmesg: move get_boot_time() to lib/timeutils
>   last: fix is_phantom() detection
>   include/c.h: add macro to print definitions as string
>   mkswap, swaplabel: move version number to header
>   mkswap: remove legacy swap structure
>   include/swapheader.h: ensure type sizes
>   swapon: swaps with legacy version label are not supported
>   swapon, swapheader, mkswap: move swap signature to header
>   libsmartcols: remove ununsed assignment
>   lib/timeutils: fix memory leak
>   lib/pager: use names when referring to standard file descriptors

Good evening,

I got rid of the xalloc change, with intention to send some sort of
xrecalloc() later. The 'swapon: swaps with legacy version label are
not supported' patch now uses SWAP_SIGNATURE_SZ instead of magic
number & Bernhard is added as reviewer. Karel, I am not sure if I got
the libsmartcols check right so please ensure the logic is correct.

Corrected versions of the patches are available from my github remote
branch that is using the same name as previous time I sent these. IMHO
the changes are pretty small, so resubmission is not (yet) necessary.
If there's more feedback that might change.

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

Re: [PATCH 00/12] pull: mostly swap command clarifications

[Bernhard Voelker]

On 04/29/2014 11:05 PM, Sami Kerola wrote:
> The 'swapon: swaps with legacy version label are
> not supported' patch now uses SWAP_SIGNATURE_SZ instead of magic
> number & Bernhard is added as reviewer.
___________^^^^^^^^_____________^^^^^^^^

s/Bernhard/Benno/

Have a nice day,
Berny

Re: [PATCH 00/12] pull: mostly swap command clarifications

[Karel Zak]

On Sun, Apr 27, 2014 at 09:05:26PM +0100, Sami Kerola wrote:
>   git://github.com/kerolasa/lelux-utiliteetit.git 2014wk16

 Merged with some minor changes.

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