[PATCH 00/13] pull: warnings fixes and blkid translations

[PATCH 00/13] pull: warnings fixes and blkid translations

[Sami Kerola]

Hello,

Following few changes are various small fixes.  Many of the changes are
about warnings, and not that interesting.  Perhaps the most unexpected
change is blkid translations.  I don't know how that command has slipped
through so long time without being part of translations.  Oh well, now that
is fixed.


----------------------------------------------------------------
The following changes since commit 1765814521bf2aba7b8c03d3ab2e3b784e1ce2c4:
  libsmartcols: allow to add column to already used table (2017-03-03 10:37:37 +0100)
are available in the git repository at:
  git://github.com/kerolasa/lelux-utiliteetit.git 2017wk09
for you to fetch changes up to b8711cb7f2e0f739a491b6a95e7fb8dc537d977f:
  lib/colors: assert rathen than overflow (2017-03-05 20:41:41 +0000)
----------------------------------------------------------------

Sami Kerola (13):
  misc: stop mixing declarations and code
  libmount: fix use of preprocessor identifier
  libblkid: avoid overflow when initializing array
  libblkid: ensure uninitialized variable is not used [clang]
  blkid: add curly braces to do-while statement
  blkid: add messages to translations
  blkid: define output names only once
  blkid: improve coding style
  include: fix compiler warning
  libfdisk: dos: fix potential null pointer dereferences
  libfdisk: fix potentially null
  docs: improve boilerplate.c options example
  lib/colors: assert rathen than overflow

 Documentation/boilerplate.c           |  13 +-
 disk-utils/cfdisk.c                   |   4 +-
 disk-utils/sfdisk.c                   |   2 +-
 include/optutils.h                    |   3 +
 include/pt-mbr.h                      |   3 +
 lib/colors.c                          |   4 +-
 libblkid/src/superblocks/drbdmanage.c |   2 +-
 libblkid/src/superblocks/minix.c      |  22 +--
 libfdisk/src/dos.c                    |  61 +++++++-
 libfdisk/src/partition.c              |   4 +
 libmount/src/context_mount.c          |   2 +-
 misc-utils/blkid.c                    | 269 ++++++++++++++++++----------------
 text-utils/hexdump-display.c          |   4 +-
 13 files changed, 241 insertions(+), 152 deletions(-)

-- 
2.12.0

[PATCH 01/13] misc: stop mixing declarations and code

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/cfdisk.c              | 4 +++-
 disk-utils/sfdisk.c              | 2 +-
 libblkid/src/superblocks/minix.c | 7 +++----
 text-utils/hexdump-display.c     | 4 ++--
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/disk-utils/cfdisk.c b/disk-utils/cfdisk.c
index f06dad1c8..cb9f3421d 100644
--- a/disk-utils/cfdisk.c
+++ b/disk-utils/cfdisk.c
@@ -2092,6 +2092,8 @@ static int ui_create_label(struct cfdisk *cf)
 
 
 	do {
+		int key;
+
 		if (refresh_menu) {
 			ui_draw_menu(cf);
 			ui_hint(_("Select a type to create a new label or press 'L' to load script file."));
@@ -2099,7 +2101,7 @@ static int ui_create_label(struct cfdisk *cf)
 			refresh_menu = 0;
 		}
 
-		int key = getch();
+		key = getch();
 
 		if (ui_resize)
 			ui_menu_resize(cf);
diff --git a/disk-utils/sfdisk.c b/disk-utils/sfdisk.c
index f4f9b6b1a..cf5dd0052 100644
--- a/disk-utils/sfdisk.c
+++ b/disk-utils/sfdisk.c
@@ -1495,7 +1495,7 @@ static void follow_wipe_mode(struct sfdisk *sf)
 static int wipe_partition(struct sfdisk *sf, size_t partno)
 {
 	int rc, yes = 0;
-	char *fstype = NULL;;
+	char *fstype = NULL;
 	struct fdisk_partition *tmp = NULL;
 
 	DBG(MISC, ul_debug("checking for signature"));
diff --git a/libblkid/src/superblocks/minix.c b/libblkid/src/superblocks/minix.c
index feebc96ad..b8881e574 100644
--- a/libblkid/src/superblocks/minix.c
+++ b/libblkid/src/superblocks/minix.c
@@ -77,6 +77,9 @@ static int probe_minix(blkid_probe pr,
 	unsigned char *ext;
 	const unsigned char *data;
 	int version = 0, swabme = 0;
+	unsigned long zones, ninodes, imaps, zmaps;
+	off_t firstz;
+	size_t zone_size;
 
 	data = blkid_probe_get_buffer(pr, 1024,
 			max(sizeof(struct minix_super_block),
@@ -87,10 +90,6 @@ static int probe_minix(blkid_probe pr,
 	if (version < 1)
 		return 1;
 
-	unsigned long zones, ninodes, imaps, zmaps;
-	off_t firstz;
-	size_t zone_size;
-
 	if (version <= 2) {
 		struct minix_super_block *sb = (struct minix_super_block *) data;
 
diff --git a/text-utils/hexdump-display.c b/text-utils/hexdump-display.c
index ce714f68c..3e017635b 100644
--- a/text-utils/hexdump-display.c
+++ b/text-utils/hexdump-display.c
@@ -299,9 +299,9 @@ void display(struct hexdump *hex)
 			eaddress = address;
 		}
 		list_for_each (p, &endfu->prlist) {
-			pr = list_entry(p, struct hexdump_pr, prlist);
-
 			const char *color = NULL;
+
+			pr = list_entry(p, struct hexdump_pr, prlist);
 			if (colors_wanted() && pr->colorlist
 			    && (color = color_cond(pr, bp, pr->bcnt))) {
 				color_enable(color);
-- 
2.12.0

[PATCH 02/13] libmount: fix use of preprocessor identifier

[Sami Kerola]

libmount/src/context_mount.c:990:5: warning: undefined preprocessor
identifier 'USE_LIBMOUNT_SUPPORT_MTAB'

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

diff --git a/libmount/src/context_mount.c b/libmount/src/context_mount.c
index 23f0e303a..6368e9ba8 100644
--- a/libmount/src/context_mount.c
+++ b/libmount/src/context_mount.c
@@ -987,7 +987,7 @@ int mnt_context_do_mount(struct libmnt_context *cxt)
 	} else
 		res = do_mount_by_pattern(cxt, cxt->fstype_pattern);
 
-#if USE_LIBMOUNT_SUPPORT_MTAB
+#ifdef USE_LIBMOUNT_SUPPORT_MTAB
 	if (mnt_context_get_status(cxt)
 	    && !mnt_context_is_fake(cxt)
 	    && !cxt->helper) {
-- 
2.12.0

[PATCH 03/13] libblkid: avoid overflow when initializing array

[Sami Kerola]

libblkid/src/superblocks/drbdmanage.c:38:42: warning: too long
initializer-string for array of char(no space for nul char)

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

diff --git a/libblkid/src/superblocks/drbdmanage.c b/libblkid/src/superblocks/drbdmanage.c
index 48bf1dabf..b45f89bc8 100644
--- a/libblkid/src/superblocks/drbdmanage.c
+++ b/libblkid/src/superblocks/drbdmanage.c
@@ -35,7 +35,7 @@ struct drbdmanage_pers {
 } __attribute__ ((packed));
 
 
-static const char persistence_magic[4] = "\x1a\xdb\x98\xa2";
+static const char persistence_magic[4] = { '\x1a', '\xdb', '\x98', '\xa2' };
 
 
 static int probe_drbdmanage(blkid_probe pr,
-- 
2.12.0

[PATCH 04/13] libblkid: ensure uninitialized variable is not used [clang]

[Sami Kerola]

Following condition can be true if minix file system is corrupt, and versio
number is found to be greater than 3.  It is fair to say described scenario
is unlikely.

libblkid/src/superblocks/minix.c:107:13: warning: variable 'zone_size' is
used uninitialized whenever
      'if' condition is false [-Wsometimes-uninitialized]
        } else if (version == 3) {
                   ^~~~~~~~~~~~
libblkid/src/superblocks/minix.c:121:6: note: uninitialized use occurs here
        if (zone_size != 0 || ninodes == 0 || ninodes == UINT32_MAX)
            ^~~~~~~~~

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 libblkid/src/superblocks/minix.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/libblkid/src/superblocks/minix.c b/libblkid/src/superblocks/minix.c
index b8881e574..c47378d73 100644
--- a/libblkid/src/superblocks/minix.c
+++ b/libblkid/src/superblocks/minix.c
@@ -87,10 +87,9 @@ static int probe_minix(blkid_probe pr,
 	if (!data)
 		return errno ? -errno : 1;
 	version = get_minix_version(data, &swabme);
-	if (version < 1)
-		return 1;
-
-	if (version <= 2) {
+	switch (version) {
+	case 1:
+	case 2: {
 		struct minix_super_block *sb = (struct minix_super_block *) data;
 
 		uint16_t state = minix_swab16(swabme, sb->s_state);
@@ -104,7 +103,9 @@ static int probe_minix(blkid_probe pr,
 		zmaps   = minix_swab16(swabme, sb->s_zmap_blocks);
 		firstz  = minix_swab16(swabme, sb->s_firstdatazone);
 		zone_size = sb->s_log_zone_size;
-	} else if (version == 3) {
+		break;
+	}
+	case 3: {
 		struct minix3_super_block *sb = (struct minix3_super_block *) data;
 
 		zones = minix_swab32(swabme, sb->s_zones);
@@ -113,6 +114,10 @@ static int probe_minix(blkid_probe pr,
 		zmaps   = minix_swab16(swabme, sb->s_zmap_blocks);
 		firstz  = minix_swab16(swabme, sb->s_firstdatazone);
 		zone_size = sb->s_log_zone_size;
+		break;
+	}
+	default:
+		return 1;
 	}
 
 	/* sanity checks to be sure that the FS is really minix.
-- 
2.12.0

[PATCH 05/13] blkid: add curly braces to do-while statement

[Sami Kerola]

Some say people read do-while statements are easily as while statements and
get wrong impression.  This happens because do-whiles are not common.
Adding braces should make it easier to read the code.

Reference: http://www.spinics.net/lists/linux-kernel-janitors/msg02018.html
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/blkid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/misc-utils/blkid.c b/misc-utils/blkid.c
index 9278553c7..7bbb54dfd 100644
--- a/misc-utils/blkid.c
+++ b/misc-utils/blkid.c
@@ -139,9 +139,9 @@ static int pretty_print_word(const char *str, int max_len,
 		len = 0;
 	} else if (len > max_len)
 		ret = len - max_len;
-	do
+	do {
 		fputc(' ', stdout);
-	while (len++ < max_len);
+	} while (len++ < max_len);
 	return ret;
 }
 
-- 
2.12.0

[PATCH 06/13] blkid: add messages to translations

[Sami Kerola]

Without obvious reason blkid has managed to be part of this project for
pretty long time without getting translations.  Lets change that.  In same
go use the usual error printing facilities instead of fprintf(stderr, ...);

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/blkid.c | 131 ++++++++++++++++++++++++++---------------------------
 1 file changed, 65 insertions(+), 66 deletions(-)

diff --git a/misc-utils/blkid.c b/misc-utils/blkid.c
index 7bbb54dfd..4746e09a7 100644
--- a/misc-utils/blkid.c
+++ b/misc-utils/blkid.c
@@ -43,6 +43,7 @@
 #define CLOSE_EXIT_CODE		BLKID_EXIT_OTHER	/* close_stdout() */
 #include "closestream.h"
 
+#include "nls.h"
 #include "ttyutils.h"
 #include "xalloc.h"
 
@@ -50,7 +51,7 @@ static int raw_chars;
 
 static void print_version(FILE *out)
 {
-	fprintf(out, "%s from %s  (libblkid %s, %s)\n",
+	fprintf(out, _("%s from %s  (libblkid %s, %s)\n"),
 		program_invocation_short_name, PACKAGE_STRING,
 		LIBBLKID_VERSION, LIBBLKID_DATE);
 }
@@ -60,38 +61,38 @@ static void usage(int error)
 	FILE *out = error ? stderr : stdout;
 
 	print_version(out);
-	fprintf(out,
-		"Usage:\n"
-		" %1$s -L <label> | -U <uuid>\n\n"
-		" %1$s [-c <file>] [-ghlLv] [-o <format>] [-s <tag>] \n"
-		"       [-t <token>] [<dev> ...]\n\n"
-		" %1$s -p [-s <tag>] [-O <offset>] [-S <size>] \n"
-		"       [-o <format>] <dev> ...\n\n"
-		" %1$s -i [-s <tag>] [-o <format>] <dev> ...\n\n"
-		"Options:\n"
-		" -c <file>   read from <file> instead of reading from the default\n"
-		"               cache file (-c /dev/null means no cache)\n"
-		" -d          don't encode non-printing characters\n"
-		" -h          print this usage message and exit\n"
-		" -g          garbage collect the blkid cache\n"
-		" -o <format> output format; can be one of:\n"
-		"               value, device, export or full; (default: full)\n"
-		" -k          list all known filesystems/RAIDs and exit\n"
-		" -s <tag>    show specified tag(s) (default show all tags)\n"
-		" -t <token>  find device with a specific token (NAME=value pair)\n"
-		" -l          look up only first device with token specified by -t\n"
-		" -L <label>  convert LABEL to device name\n"
-		" -U <uuid>   convert UUID to device name\n"
-		" -V          print version and exit\n"
-		" <dev>       specify device(s) to probe (default: all devices)\n\n"
-		"Low-level probing options:\n"
-		" -p          low-level superblocks probing (bypass cache)\n"
-		" -i          gather information about I/O limits\n"
-		" -S <size>   overwrite device size\n"
-		" -O <offset> probe at the given offset\n"
-		" -u <list>   filter by \"usage\" (e.g. -u filesystem,raid)\n"
-		" -n <list>   filter by filesystem type (e.g. -n vfat,ext3)\n"
-		"\n", program_invocation_short_name);
+	fputs(USAGE_HEADER, out);
+	fprintf(out, _(	" %s -L <label> | -U <uuid>\n\n"), program_invocation_short_name);
+	fprintf(out, _(	" %s [-c <file>] [-ghlLv] [-o <format>] [-s <tag>] \n"
+			"       [-t <token>] [<dev> ...]\n\n"), program_invocation_short_name);
+	fprintf(out, _(	" %s -p [-s <tag>] [-O <offset>] [-S <size>] \n"
+			"       [-o <format>] <dev> ...\n\n"), program_invocation_short_name);
+	fprintf(out, _(	" %s -i [-s <tag>] [-o <format>] <dev> ...\n\n"), program_invocation_short_name);
+	fputs(USAGE_OPTIONS, out);
+	fputs(_(	" -c <file>   read from <file> instead of reading from the default\n"
+			"               cache file (-c /dev/null means no cache)\n"), out);
+	fputs(_(	" -d          don't encode non-printing characters\n"), out);
+	fputs(_(	" -h          print this usage message and exit\n"), out);
+	fputs(_(	" -g          garbage collect the blkid cache\n"), out);
+	fputs(_(	" -o <format> output format; can be one of:\n"
+			"               value, device, export or full; (default: full)\n"), out);
+	fputs(_(	" -k          list all known filesystems/RAIDs and exit\n"), out);
+	fputs(_(	" -s <tag>    show specified tag(s) (default show all tags)\n"), out);
+	fputs(_(	" -t <token>  find device with a specific token (NAME=value pair)\n"), out);
+	fputs(_(	" -l          look up only first device with token specified by -t\n"), out);
+	fputs(_(	" -L <label>  convert LABEL to device name\n"), out);
+	fputs(_(	" -U <uuid>   convert UUID to device name\n"), out);
+	fputs(_(	" -V          print version and exit\n"), out);
+	fputs(_(	" <dev>       specify device(s) to probe (default: all devices)\n"), out);
+	fputs(          "\n", out);
+	fputs(_(	"Low-level probing options:\n"), out);
+	fputs(_(	" -p          low-level superblocks probing (bypass cache)\n"), out);
+	fputs(_(	" -i          gather information about I/O limits\n"), out);
+	fputs(_(	" -S <size>   overwrite device size\n"), out);
+	fputs(_(	" -O <offset> probe at the given offset\n"), out);
+	fputs(_(	" -u <list>   filter by \"usage\" (e.g. -u filesystem,raid)\n"), out);
+	fputs(_(	" -n <list>   filter by filesystem type (e.g. -n vfat,ext3)\n"), out);
+	fputs(		"\n", out);
 
 	exit(error);
 }
@@ -219,11 +220,11 @@ static void pretty_print_dev(blkid_dev dev)
 	if (retval == 0) {
 		if (mount_flags & MF_MOUNTED) {
 			if (!mtpt[0])
-				strcpy(mtpt, "(mounted, mtpt unknown)");
+				strcpy(mtpt, _("(mounted, mtpt unknown)"));
 		} else if (mount_flags & MF_BUSY)
-			strcpy(mtpt, "(in use)");
+			strcpy(mtpt, _("(in use)"));
 		else
-			strcpy(mtpt, "(not mounted)");
+			strcpy(mtpt, _("(not mounted)"));
 	}
 
 	pretty_print_line(devname, fs_type, label, mtpt, uuid);
@@ -487,7 +488,7 @@ static int lowprobe_device(blkid_probe pr, const char *devname,
 
 	fd = open(devname, O_RDONLY|O_CLOEXEC);
 	if (fd < 0) {
-		fprintf(stderr, "error: %s: %m\n", devname);
+		warn(_("error: %s"), devname);
 		return BLKID_EXIT_NOTFOUND;
 	}
 	if (blkid_probe_set_device(pr, fd, offset, size))
@@ -531,10 +532,9 @@ done:
 		if (output & OUTPUT_UDEV_LIST)
 			print_udev_ambivalent(pr);
 		else
-			fprintf(stderr,
-				"%s: ambivalent result (probably more "
+			warnx(_("%s: ambivalent result (probably more "
 				"filesystems on the device, use wipefs(8) "
-				"to see more details)\n",
+				"to see more details)"),
 				devname);
 	}
 	close(fd);
@@ -578,7 +578,7 @@ static int list_to_usage(const char *list, int *flag)
 	return mask;
 err:
 	*flag = 0;
-	fprintf(stderr, "unknown keyword in -u <list> argument: '%s'\n",
+	warnx(_("unknown keyword in -u <list> argument: '%s'"),
 			word ? word : list);
 	exit(BLKID_EXIT_OTHER);
 }
@@ -595,7 +595,7 @@ static char **list_to_types(const char *list, int *flag)
 		p += 2;
 	}
 	if (!p || !*p) {
-		fprintf(stderr, "error: -u <list> argument is empty\n");
+		warnx(_("error: -u <list> argument is empty"));
 		goto err;
 	}
 	for (i = 1; p && (p = strchr(p, ',')); i++, p++);
@@ -656,6 +656,9 @@ int main(int argc, char **argv)
 	int excl_st[ARRAY_SIZE(excl)] = UL_EXCL_STATUS_INIT;
 
 	show[0] = NULL;
+	setlocale(LC_ALL, "");
+	bindtextdomain(PACKAGE, LOCALEDIR);
+	textdomain(PACKAGE);
 	atexit(close_stdout);
 
 	while ((c = getopt (argc, argv,
@@ -721,39 +724,38 @@ int main(int argc, char **argv)
 			else if (!strcmp(optarg, "full"))
 				output_format = 0;
 			else {
-				fprintf(stderr, "Invalid output format %s. "
+				errx(BLKID_EXIT_OTHER, _("Invalid output format %s. "
 					"Choose from value,\n\t"
-					"device, list, udev or full\n", optarg);
-				exit(BLKID_EXIT_OTHER);
+					"value, device, list, udev, export, or full"), optarg);
 			}
 			break;
 		case 'O':
-			offset = strtosize_or_err(optarg, "invalid offset argument");
+			offset = strtosize_or_err(optarg, _("invalid offset argument"));
 			break;
 		case 'p':
 			lowprobe |= LOWPROBE_SUPERBLOCKS;
 			break;
 		case 's':
 			if (numtag + 1 >= sizeof(show) / sizeof(*show)) {
-				fprintf(stderr, "Too many tags specified\n");
+				warnx(_("Too many tags specified"));
 				usage(err);
 			}
 			show[numtag++] = optarg;
 			show[numtag] = NULL;
 			break;
 		case 'S':
-			size = strtosize_or_err(optarg, "invalid size argument");
+			size = strtosize_or_err(optarg, _("invalid size argument"));
 			break;
 		case 't':
 			if (search_type) {
-				fprintf(stderr, "Can only search for "
-						"one NAME=value pair\n");
+				warnx(_("Can only search for "
+					"one NAME=value pair"));
 				usage(err);
 			}
 			if (blkid_parse_tag_string(optarg,
 						   &search_type,
 						   &search_value)) {
-				fprintf(stderr, "-t needs NAME=value pair\n");
+				warnx(_("-t needs NAME=value pair"));
 				usage(err);
 			}
 			break;
@@ -803,11 +805,10 @@ int main(int argc, char **argv)
 	err = BLKID_EXIT_NOTFOUND;
 
 	if (eval == 0 && (output_format & OUTPUT_PRETTY_LIST)) {
-		if (lowprobe) {
-			fprintf(stderr, "The low-level probing mode does not "
-					"support 'list' output format\n");
-			exit(BLKID_EXIT_OTHER);
-		}
+		if (lowprobe)
+			errx(BLKID_EXIT_OTHER,
+			     _("The low-level probing mode does not "
+			       "support 'list' output format"));
 		pretty_print_dev(NULL);
 	}
 
@@ -817,11 +818,10 @@ int main(int argc, char **argv)
 		 */
 		blkid_probe pr;
 
-		if (!numdev) {
-			fprintf(stderr, "The low-level probing mode "
-					"requires a device\n");
-			exit(BLKID_EXIT_OTHER);
-		}
+		if (!numdev)
+			errx(BLKID_EXIT_OTHER,
+			     _("The low-level probing mode "
+			       "requires a device"));
 
 		/* automatically enable 'export' format for I/O Limits */
 		if (!output_format  && (lowprobe & LOWPROBE_TOPOLOGY))
@@ -870,11 +870,10 @@ int main(int argc, char **argv)
 		 */
 		blkid_dev dev;
 
-		if (!search_type) {
-			fprintf(stderr, "The lookup option requires a "
-				"search type specified using -t\n");
-			exit(BLKID_EXIT_OTHER);
-		}
+		if (!search_type)
+			errx(BLKID_EXIT_OTHER,
+			     _("The lookup option requires a "
+			       "search type specified using -t"));
 		/* Load any additional devices not in the cache */
 		for (i = 0; i < numdev; i++)
 			blkid_get_dev(cache, devices[i], BLKID_DEV_NORMAL);
-- 
2.12.0

Re: [PATCH 06/13] blkid: add messages to translations

[J William Piggott]

On 03/05/2017 03:52 PM, Sami Kerola wrote:

 8< ---

> +	fputs(_(	" -o <format> output format; can be one of:\n"
> +			"               value, device, export or full; (default: full)\n"), out);

	This says there are four output formats, but the error message
	below lists six?

 8< ---

> @@ -721,39 +724,38 @@ int main(int argc, char **argv)
>  			else if (!strcmp(optarg, "full"))
>  				output_format = 0;
>  			else {
> -				fprintf(stderr, "Invalid output format %s. "
> +				errx(BLKID_EXIT_OTHER, _("Invalid output format %s. "
>  					"Choose from value,\n\t"
> -					"device, list, udev or full\n", optarg);
> -				exit(BLKID_EXIT_OTHER);
> +					"value, device, list, udev, export, or full"), optarg);


 8< ---


Looks like the above outputs 'value' twice?

Maybe it should also have a colon "Choose from:\n\t"

A single line would be preferable I think:
"Choose from: value, device, list, udev, export, or full"

Re: [PATCH 06/13] blkid: add messages to translations

[Karel Zak]

On Tue, Mar 07, 2017 at 09:15:25AM -0500, J William Piggott wrote:
> 
> 
> On 03/05/2017 03:52 PM, Sami Kerola wrote:
> 
>  8< ---
> 
> > +	fputs(_(	" -o <format> output format; can be one of:\n"
> > +			"               value, device, export or full; (default: full)\n"), out);
> 
> 	This says there are four output formats, but the error message
> 	below lists six?
> 
>  8< ---
> 
> > @@ -721,39 +724,38 @@ int main(int argc, char **argv)
> >  			else if (!strcmp(optarg, "full"))
> >  				output_format = 0;
> >  			else {
> > -				fprintf(stderr, "Invalid output format %s. "
> > +				errx(BLKID_EXIT_OTHER, _("Invalid output format %s. "
> >  					"Choose from value,\n\t"
> > -					"device, list, udev or full\n", optarg);
> > -				exit(BLKID_EXIT_OTHER);
> > +					"value, device, list, udev, export, or full"), optarg);
> 
> 
>  8< ---
> 
> 
> Looks like the above outputs 'value' twice?
> 
> Maybe it should also have a colon "Choose from:\n\t"
> 
> A single line would be preferable I think:
> "Choose from: value, device, list, udev, export, or full"

 The message is unnecessary complex, the list of the supported
 arguments is overkill. I have merged:

    errx(BLKID_EXIT_OTHER, _("unsupported output format %s"), optarg);

    Karel

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

Re: [PATCH 06/13] blkid: add messages to translations

[Karel Zak]

On Sun, Mar 05, 2017 at 08:52:25PM +0000, Sami Kerola wrote:
>  	while ((c = getopt (argc, argv,
> @@ -721,39 +724,38 @@ int main(int argc, char **argv)
>  			else if (!strcmp(optarg, "full"))
>  				output_format = 0;
>  			else {
> -				fprintf(stderr, "Invalid output format %s. "
> +				errx(BLKID_EXIT_OTHER, _("Invalid output format %s. "
>  					"Choose from value,\n\t"
> -					"device, list, udev or full\n", optarg);
> -				exit(BLKID_EXIT_OTHER);
> +					"value, device, list, udev, export, or full"), optarg);
>  			}
>  			break;
>  		case 'O':
> -			offset = strtosize_or_err(optarg, "invalid offset argument");
> +			offset = strtosize_or_err(optarg, _("invalid offset argument"));
>  			break;
>  		case 'p':
>  			lowprobe |= LOWPROBE_SUPERBLOCKS;
>  			break;
>  		case 's':
>  			if (numtag + 1 >= sizeof(show) / sizeof(*show)) {
> -				fprintf(stderr, "Too many tags specified\n");
> +				warnx(_("Too many tags specified"));
>  				usage(err);

 Ah, it seems blkid is one of the remaining tools where I have no
 changed usage(err) to errtryh(err).

 We don't want to print all usage() on errors, it's better to see
 
    Try 'blkid -h' for more information.

 only.


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

[PATCH 07/13] blkid: define output names only once

[Sami Kerola]

Output names are magic strings, and they should be defined only once in one
place to avoid mismatches and/or incompleteness.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/blkid.c | 104 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 63 insertions(+), 41 deletions(-)

diff --git a/misc-utils/blkid.c b/misc-utils/blkid.c
index 4746e09a7..887a80083 100644
--- a/misc-utils/blkid.c
+++ b/misc-utils/blkid.c
@@ -19,11 +19,23 @@
 #include <errno.h>
 #include <getopt.h>
 
-#define OUTPUT_VALUE_ONLY	(1 << 1)
-#define OUTPUT_DEVICE_ONLY	(1 << 2)
-#define OUTPUT_PRETTY_LIST	(1 << 3)		/* deprecated */
-#define OUTPUT_UDEV_LIST	(1 << 4)		/* deprecated */
-#define OUTPUT_EXPORT_LIST	(1 << 5)
+enum {
+	OUTPUT_FULL = 0,
+	OUTPUT_VALUE_ONLY,
+	OUTPUT_DEVICE_ONLY,
+	OUTPUT_PRETTY_LIST,
+	OUTPUT_UDEV_LIST,
+	OUTPUT_EXPORT_LIST
+};
+
+static const char *output_names[] = {
+	[OUTPUT_FULL]		= "full",
+	[OUTPUT_VALUE_ONLY]	= "value",
+	[OUTPUT_DEVICE_ONLY]	= "device",
+	[OUTPUT_PRETTY_LIST]	= "list",	/* deprecated */
+	[OUTPUT_UDEV_LIST]	= "udev",	/* deprecated */
+	[OUTPUT_EXPORT_LIST]	= "export"
+};
 
 #define LOWPROBE_TOPOLOGY	(1 << 1)
 #define LOWPROBE_SUPERBLOCKS	(1 << 2)
@@ -97,6 +109,17 @@ static void usage(int error)
 	exit(error);
 }
 
+static int parse_output_name(const char *arg)
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(output_names); i++) {
+		if (strcmp(output_names[i], arg) == 0)
+			return i;
+	}
+	return -EINVAL;
+}
+
 /*
  * This function does "safe" printing.  It will convert non-printable
  * ASCII characters using '^' and M- notation.
@@ -288,22 +311,23 @@ static int has_item(char *ary[], const char *item)
 static void print_value(int output, int num, const char *devname,
 			const char *value, const char *name, size_t valsz)
 {
-	if (output & OUTPUT_VALUE_ONLY) {
+	switch (output) {
+	case OUTPUT_VALUE_ONLY:
 		fputs(value, stdout);
 		fputc('\n', stdout);
-
-	} else if (output & OUTPUT_UDEV_LIST) {
+		break;
+	case OUTPUT_UDEV_LIST:
 		print_udev_format(name, value);
-
-	} else if (output & OUTPUT_EXPORT_LIST) {
+		break;
+	case OUTPUT_EXPORT_LIST:
 		if (num == 1 && devname)
 			printf("DEVNAME=%s\n", devname);
 		fputs(name, stdout);
 		fputs("=", stdout);
 		safe_print(value, valsz, " \\\"'$`<>");
 		fputs("\n", stdout);
-
-	} else {
+		break;
+	default:
 		if (num == 1 && devname)
 			printf("%s:", devname);
 		fputs(" ", stdout);
@@ -324,14 +348,14 @@ static void print_tags(blkid_dev dev, char *show[], int output)
 	if (!dev)
 		return;
 
-	if (output & OUTPUT_PRETTY_LIST) {
+	if (output == OUTPUT_PRETTY_LIST) {
 		pretty_print_dev(dev);
 		return;
 	}
 
 	devname = blkid_dev_devname(dev);
 
-	if (output & OUTPUT_DEVICE_ONLY) {
+	if (output == OUTPUT_DEVICE_ONLY) {
 		printf("%s\n", devname);
 		return;
 	}
@@ -342,7 +366,7 @@ static void print_tags(blkid_dev dev, char *show[], int output)
 			continue;
 
 		if (num == 1 && !first &&
-		    (output & (OUTPUT_UDEV_LIST | OUTPUT_EXPORT_LIST)))
+		    (output == OUTPUT_UDEV_LIST || output == OUTPUT_EXPORT_LIST))
 			/* add extra line between output from more devices */
 			fputc('\n', stdout);
 
@@ -351,8 +375,8 @@ static void print_tags(blkid_dev dev, char *show[], int output)
 	blkid_tag_iterate_end(iter);
 
 	if (num > 1) {
-		if (!(output & (OUTPUT_VALUE_ONLY | OUTPUT_UDEV_LIST |
-						OUTPUT_EXPORT_LIST)))
+		if (!(output == OUTPUT_VALUE_ONLY || output == OUTPUT_UDEV_LIST
+		      || output == OUTPUT_EXPORT_LIST))
 			printf("\n");
 		first = 0;
 	}
@@ -504,11 +528,11 @@ static int lowprobe_device(blkid_probe pr, const char *devname,
 	if (!rc)
 		nvals = blkid_probe_numof_values(pr);
 
-	if (nvals && !first && output & (OUTPUT_UDEV_LIST | OUTPUT_EXPORT_LIST))
+	if (nvals && !first && (output == OUTPUT_UDEV_LIST || output == OUTPUT_EXPORT_LIST))
 		/* add extra line between output from devices */
 		fputc('\n', stdout);
 
-	if (nvals && (output & OUTPUT_DEVICE_ONLY)) {
+	if (nvals && output == OUTPUT_DEVICE_ONLY) {
 		printf("%s\n", devname);
 		goto done;
 	}
@@ -524,12 +548,15 @@ static int lowprobe_device(blkid_probe pr, const char *devname,
 
 	if (first)
 		first = 0;
-	if (nvals >= 1 && !(output & (OUTPUT_VALUE_ONLY |
-					OUTPUT_UDEV_LIST | OUTPUT_EXPORT_LIST)))
+
+	if (nvals >= 1 && !(output == OUTPUT_VALUE_ONLY ||
+			    output == OUTPUT_UDEV_LIST ||
+			    output == OUTPUT_EXPORT_LIST))
 		printf("\n");
+
 done:
 	if (rc == -2) {
-		if (output & OUTPUT_UDEV_LIST)
+		if (output == OUTPUT_UDEV_LIST)
 			print_udev_ambivalent(pr);
 		else
 			warnx(_("%s: ambivalent result (probably more "
@@ -644,7 +671,7 @@ int main(int argc, char **argv)
 	int version = 0;
 	int err = BLKID_EXIT_OTHER;
 	unsigned int i;
-	int output_format = 0;
+	int output_format = OUTPUT_FULL;
 	int lookup = 0, gc = 0, lowprobe = 0, eval = 0;
 	int c;
 	uintmax_t offset = 0, size = 0;
@@ -711,22 +738,17 @@ int main(int argc, char **argv)
 			exit(EXIT_SUCCESS);
 		}
 		case 'o':
-			if (!strcmp(optarg, "value"))
-				output_format = OUTPUT_VALUE_ONLY;
-			else if (!strcmp(optarg, "device"))
-				output_format = OUTPUT_DEVICE_ONLY;
-			else if (!strcmp(optarg, "list"))
-				output_format = OUTPUT_PRETTY_LIST;	/* deprecated */
-			else if (!strcmp(optarg, "udev"))
-				output_format = OUTPUT_UDEV_LIST;
-			else if (!strcmp(optarg, "export"))
-				output_format = OUTPUT_EXPORT_LIST;
-			else if (!strcmp(optarg, "full"))
-				output_format = 0;
-			else {
-				errx(BLKID_EXIT_OTHER, _("Invalid output format %s. "
-					"Choose from value,\n\t"
-					"value, device, list, udev, export, or full"), optarg);
+			output_format = parse_output_name(optarg);
+			if (output_format < 0) {
+				size_t j;
+
+				warnx(_("Invalid output format %s. "
+					"Choose from value:"), optarg);
+				fputc('\t', stderr);
+				for (j = 0; j < ARRAY_SIZE(output_names); j++)
+					fprintf(stderr, "%s%s", j == 0 ? "" : " ", output_names[j]);
+				fputc('\n', stderr);
+				exit(BLKID_EXIT_OTHER);
 			}
 			break;
 		case 'O':
@@ -804,7 +826,7 @@ int main(int argc, char **argv)
 	}
 	err = BLKID_EXIT_NOTFOUND;
 
-	if (eval == 0 && (output_format & OUTPUT_PRETTY_LIST)) {
+	if (eval == 0 && output_format == OUTPUT_PRETTY_LIST) {
 		if (lowprobe)
 			errx(BLKID_EXIT_OTHER,
 			     _("The low-level probing mode does not "
@@ -824,7 +846,7 @@ int main(int argc, char **argv)
 			       "requires a device"));
 
 		/* automatically enable 'export' format for I/O Limits */
-		if (!output_format  && (lowprobe & LOWPROBE_TOPOLOGY))
+		if (output_format == OUTPUT_FULL && (lowprobe & LOWPROBE_TOPOLOGY))
 			output_format = OUTPUT_EXPORT_LIST;
 
 		pr = blkid_new_probe();
-- 
2.12.0

Re: [PATCH 07/13] blkid: define output names only once

[J William Piggott]

On 03/05/2017 03:52 PM, Sami Kerola wrote:

> +				warnx(_("Invalid output format %s. "
> +					"Choose from value:"), optarg);
> +				fputc('\t', stderr);
> +				for (j = 0; j < ARRAY_SIZE(output_names); j++)
> +					fprintf(stderr, "%s%s", j == 0 ? "" : " ", output_names[j]);
> +				fputc('\n', stderr);
> +				exit(BLKID_EXIT_OTHER);


I see, it is saying "Choose a value from:". 'Choose from:" is enough,
it's confusing to use 'value' when it is also one of the choices.

I also see, that 2 of the output choices are deprecated. If they are
going to be excluded from the 'help' output, shouldn't they be excluded
from the error messages as well?

Re: [PATCH 07/13] blkid: define output names only once

[J William Piggott]

On 03/07/2017 09:37 AM, J William Piggott wrote:
> 
> 
> On 03/05/2017 03:52 PM, Sami Kerola wrote:
> 
>> +				warnx(_("Invalid output format %s. "
>> +					"Choose from value:"), optarg);
>> +				fputc('\t', stderr);
>> +				for (j = 0; j < ARRAY_SIZE(output_names); j++)
>> +					fprintf(stderr, "%s%s", j == 0 ? "" : " ", output_names[j]);
>> +				fputc('\n', stderr);
>> +				exit(BLKID_EXIT_OTHER);
> 
> 
> I see, it is saying "Choose a value from:". 'Choose from:" is enough,
> it's confusing to use 'value' when it is also one of the choices.
> 
> I also see, that 2 of the output choices are deprecated. If they are
> going to be excluded from the 'help' output, shouldn't they be excluded
> from the error messages as well?

After replying yesterday something else occurred to me. This seems
unnecessarily complex for an error message. Isn't "Invalid output format %s. "
enough? Then let them either use --help or RTM?

It shouldn't be capitalized and the invalid value should be quoted:
warnx(_("invalid output format '%s'"

Re: [PATCH 07/13] blkid: define output names only once

[Sami Kerola]

On Wed, 8 Mar 2017, J William Piggott wrote:
> On 03/07/2017 09:37 AM, J William Piggott wrote:
> > On 03/05/2017 03:52 PM, Sami Kerola wrote:
> > 
> >> +				warnx(_("Invalid output format %s. "
> >> +					"Choose from value:"), optarg);
> >> +				fputc('\t', stderr);
> >> +				for (j = 0; j < ARRAY_SIZE(output_names); j++)
> >> +					fprintf(stderr, "%s%s", j == 0 ? "" : " ", output_names[j]);
> >> +				fputc('\n', stderr);
> >> +				exit(BLKID_EXIT_OTHER);
> > 
> > 
> > I see, it is saying "Choose a value from:". 'Choose from:" is enough,
> > it's confusing to use 'value' when it is also one of the choices.
> > 
> > I also see, that 2 of the output choices are deprecated. If they are
> > going to be excluded from the 'help' output, shouldn't they be excluded
> > from the error messages as well?
> 
> After replying yesterday something else occurred to me. This seems
> unnecessarily complex for an error message. Isn't "Invalid output format %s. "
> enough? Then let them either use --help or RTM?
> 
> It shouldn't be capitalized and the invalid value should be quoted:
> warnx(_("invalid output format '%s'"

Thank you for feedback JWP,

After reading what you had to say about option argument parsing I end up 
improving messaging, making soft-deprecated arguments to be hidden, and 
rewrote related functions to be something that should be relatively easy 
to make reusable for other utilities.

https://github.com/kerolasa/lelux-utiliteetit/commit/31d790e9f7fb972b34cba37caedf155afe314427

Notably I did not move list of recommended arguments to usage(). To do 
that well will require inu_use_arguments() function, that can be used 
where ever needed. But lets leave this as-is, and do these tidy 
adjustments if there it's seen these option argument functions should be 
generalized.

-- snip
commit 31d790e9f7fb972b34cba37caedf155afe314427
Author: Sami Kerola <kerolasa@iki.fi>
Date:   Tue Feb 28 22:16:58 2017 +0000

blkid: define output names only once
    
Output names are magic strings, and they should be defined only once in one
place to avoid mismatches and/or incompleteness.
    
parse_format_name() is made relatively generic, so that it can be moved to
include/optutils.h if/when needed.
    
Signed-off-by: Sami Kerola <kerolasa@iki.fi>

diff --git a/misc-utils/blkid.c b/misc-utils/blkid.c
index 2ec806c23..f6e6f977e 100644
--- a/misc-utils/blkid.c
+++ b/misc-utils/blkid.c
@@ -19,11 +19,35 @@
 #include <errno.h>
 #include <getopt.h>
 
-#define OUTPUT_VALUE_ONLY	(1 << 1)
-#define OUTPUT_DEVICE_ONLY	(1 << 2)
-#define OUTPUT_PRETTY_LIST	(1 << 3)		/* deprecated */
-#define OUTPUT_UDEV_LIST	(1 << 4)		/* deprecated */
-#define OUTPUT_EXPORT_LIST	(1 << 5)
+enum {
+	OUTPUT_FULL = 0,
+	OUTPUT_VALUE_ONLY,
+	OUTPUT_DEVICE_ONLY,
+	OUTPUT_PRETTY_LIST,
+	OUTPUT_UDEV_LIST,
+	OUTPUT_EXPORT_LIST
+};
+
+enum {
+	opt_in_use = 0,
+	opt_hide,
+	opt_deprecate,
+	opt_removed
+};
+
+struct option_arguments {
+	char *name;
+	int status;
+};
+
+static struct option_arguments output_formats[] = {
+	[OUTPUT_FULL]		= { "full", opt_in_use },
+	[OUTPUT_VALUE_ONLY]	= { "value", opt_in_use },
+	[OUTPUT_DEVICE_ONLY]	= { "device", opt_in_use },
+	[OUTPUT_PRETTY_LIST]	= { "list", opt_hide },		/* to be deprecated */
+	[OUTPUT_UDEV_LIST]	= { "udev", opt_hide },		/* to be deprecated */
+	[OUTPUT_EXPORT_LIST]	= { "export", opt_in_use },
+};
 
 #define LOWPROBE_TOPOLOGY	(1 << 1)
 #define LOWPROBE_SUPERBLOCKS	(1 << 2)
@@ -97,6 +121,36 @@ static void usage(int error)
 	exit(error);
 }
 
+static int parse_format_name(const char *arg)
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(output_formats); i++) {
+		if (strcmp(output_formats[i].name, arg) != 0)
+			continue;
+		switch (output_formats[i].status) {
+		case opt_in_use:
+		case opt_hide:
+			return i;
+		case opt_deprecate:
+			warnx(_("'%s' is deprecated"), arg);
+			return i;
+		case opt_removed:
+			warnx(_("'%s' is removed"), arg);
+			return -EINVAL;
+		default:
+			abort();
+		}
+	}
+	warnx(_("invalid argument: '%s'"), arg);
+	fprintf(stderr, _("recommended arguments to use are:"));
+	for (i = 0; i < ARRAY_SIZE(output_formats); i++)
+		if (output_formats[i].status == opt_in_use)
+			fprintf(stderr, " %s", output_formats[i].name);
+	fputc('\n', stderr);
+	return -EINVAL;
+}
+
 /*
  * This function does "safe" printing.  It will convert non-printable
  * ASCII characters using '^' and M- notation.
@@ -288,22 +342,23 @@ static int has_item(char *ary[], const char *item)
 static void print_value(int output, int num, const char *devname,
 			const char *value, const char *name, size_t valsz)
 {
-	if (output & OUTPUT_VALUE_ONLY) {
+	switch (output) {
+	case OUTPUT_VALUE_ONLY:
 		fputs(value, stdout);
 		fputc('\n', stdout);
-
-	} else if (output & OUTPUT_UDEV_LIST) {
+		break;
+	case OUTPUT_UDEV_LIST:
 		print_udev_format(name, value);
-
-	} else if (output & OUTPUT_EXPORT_LIST) {
+		break;
+	case OUTPUT_EXPORT_LIST:
 		if (num == 1 && devname)
 			printf("DEVNAME=%s\n", devname);
 		fputs(name, stdout);
 		fputs("=", stdout);
 		safe_print(value, valsz, " \\\"'$`<>");
 		fputs("\n", stdout);
-
-	} else {
+		break;
+	default:
 		if (num == 1 && devname)
 			printf("%s:", devname);
 		fputs(" ", stdout);
@@ -324,14 +379,14 @@ static void print_tags(blkid_dev dev, char *show[], int output)
 	if (!dev)
 		return;
 
-	if (output & OUTPUT_PRETTY_LIST) {
+	if (output == OUTPUT_PRETTY_LIST) {
 		pretty_print_dev(dev);
 		return;
 	}
 
 	devname = blkid_dev_devname(dev);
 
-	if (output & OUTPUT_DEVICE_ONLY) {
+	if (output == OUTPUT_DEVICE_ONLY) {
 		printf("%s\n", devname);
 		return;
 	}
@@ -342,7 +397,7 @@ static void print_tags(blkid_dev dev, char *show[], int output)
 			continue;
 
 		if (num == 1 && !first &&
-		    (output & (OUTPUT_UDEV_LIST | OUTPUT_EXPORT_LIST)))
+		    (output == OUTPUT_UDEV_LIST || output == OUTPUT_EXPORT_LIST))
 			/* add extra line between output from more devices */
 			fputc('\n', stdout);
 
@@ -351,8 +406,8 @@ static void print_tags(blkid_dev dev, char *show[], int output)
 	blkid_tag_iterate_end(iter);
 
 	if (num > 1) {
-		if (!(output & (OUTPUT_VALUE_ONLY | OUTPUT_UDEV_LIST |
-						OUTPUT_EXPORT_LIST)))
+		if (!(output == OUTPUT_VALUE_ONLY || output == OUTPUT_UDEV_LIST
+		      || output == OUTPUT_EXPORT_LIST))
 			printf("\n");
 		first = 0;
 	}
@@ -504,11 +559,11 @@ static int lowprobe_device(blkid_probe pr, const char *devname,
 	if (!rc)
 		nvals = blkid_probe_numof_values(pr);
 
-	if (nvals && !first && output & (OUTPUT_UDEV_LIST | OUTPUT_EXPORT_LIST))
+	if (nvals && !first && (output == OUTPUT_UDEV_LIST || output == OUTPUT_EXPORT_LIST))
 		/* add extra line between output from devices */
 		fputc('\n', stdout);
 
-	if (nvals && (output & OUTPUT_DEVICE_ONLY)) {
+	if (nvals && output == OUTPUT_DEVICE_ONLY) {
 		printf("%s\n", devname);
 		goto done;
 	}
@@ -524,12 +579,15 @@ static int lowprobe_device(blkid_probe pr, const char *devname,
 
 	if (first)
 		first = 0;
-	if (nvals >= 1 && !(output & (OUTPUT_VALUE_ONLY |
-					OUTPUT_UDEV_LIST | OUTPUT_EXPORT_LIST)))
+
+	if (nvals >= 1 && !(output == OUTPUT_VALUE_ONLY ||
+			    output == OUTPUT_UDEV_LIST ||
+			    output == OUTPUT_EXPORT_LIST))
 		printf("\n");
+
 done:
 	if (rc == -2) {
-		if (output & OUTPUT_UDEV_LIST)
+		if (output == OUTPUT_UDEV_LIST)
 			print_udev_ambivalent(pr);
 		else
 			warnx(_("%s: ambivalent result (probably more "
@@ -644,7 +702,7 @@ int main(int argc, char **argv)
 	int version = 0;
 	int err = BLKID_EXIT_OTHER;
 	unsigned int i;
-	int output_format = 0;
+	int output_format = OUTPUT_FULL;
 	int lookup = 0, gc = 0, lowprobe = 0, eval = 0;
 	int c;
 	uintmax_t offset = 0, size = 0;
@@ -711,23 +769,9 @@ int main(int argc, char **argv)
 			exit(EXIT_SUCCESS);
 		}
 		case 'o':
-			if (!strcmp(optarg, "value"))
-				output_format = OUTPUT_VALUE_ONLY;
-			else if (!strcmp(optarg, "device"))
-				output_format = OUTPUT_DEVICE_ONLY;
-			else if (!strcmp(optarg, "list"))
-				output_format = OUTPUT_PRETTY_LIST;	/* deprecated */
-			else if (!strcmp(optarg, "udev"))
-				output_format = OUTPUT_UDEV_LIST;
-			else if (!strcmp(optarg, "export"))
-				output_format = OUTPUT_EXPORT_LIST;
-			else if (!strcmp(optarg, "full"))
-				output_format = 0;
-			else {
-				errx(BLKID_EXIT_OTHER, _("Invalid output format %s. "
-					"Choose from value,\n\t"
-					"device, list, udev or full\n", optarg);
-			}
+			output_format = parse_format_name(optarg);
+			if (output_format < 0)
+				exit(BLKID_EXIT_OTHER);
 			break;
 		case 'O':
 			offset = strtosize_or_err(optarg, _("invalid offset argument"));
@@ -804,7 +848,7 @@ int main(int argc, char **argv)
 	}
 	err = BLKID_EXIT_NOTFOUND;
 
-	if (eval == 0 && (output_format & OUTPUT_PRETTY_LIST)) {
+	if (eval == 0 && output_format == OUTPUT_PRETTY_LIST) {
 		if (lowprobe)
 			errx(BLKID_EXIT_OTHER,
 			     _("The low-level probing mode does not "
@@ -824,7 +868,7 @@ int main(int argc, char **argv)
 			       "requires a device"));
 
 		/* automatically enable 'export' format for I/O Limits */
-		if (!output_format  && (lowprobe & LOWPROBE_TOPOLOGY))
+		if (output_format == OUTPUT_FULL && (lowprobe & LOWPROBE_TOPOLOGY))
 			output_format = OUTPUT_EXPORT_LIST;
 
 		pr = blkid_new_probe();

Re: [PATCH 07/13] blkid: define output names only once

[Sami Kerola]

On Fri, 10 Mar 2017, Sami Kerola wrote:
> Notably I did not move list of recommended arguments to usage(). To do 
> that well will require inu_use_arguments() function, that can be used 
> where ever needed. But lets leave this as-is, and do these tidy 
> adjustments if there it's seen these option argument functions should be 
> generalized.

Hi again,

Having day off and idea to generalize this did not go away from my mind, 
so I decided to give it a shot. I'm not sure how many utils might need 
this, but that is hardly a reason against or in favour to include this. 
Better question is if this is the way argument lists should be managed 
when they are deprecated.

The below change can also be found from
  git://github.com/kerolasa/lelux-utiliteetit.git 2017wk09

--->8----
From: Sami Kerola <kerolasa@iki.fi>
Date: Fri, 10 Mar 2017 14:06:10 +0000
Subject: [PATCH] include/optutils: add option argument parsing and printing functions

Functions parse_arg_name() and print_valid_args() add methods that make
option argument life cycle management easy.

Based-on-feedback-from: J William Piggott <elseifthen@gmx.com>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/optutils.h | 50 +++++++++++++++++++++++++++++++
 misc-utils/blkid.c | 87 
+++++++++++++++---------------------------------------
 2 files changed, 73 insertions(+), 64 deletions(-)

diff --git a/include/optutils.h b/include/optutils.h
index 325cb8812..62ccc90ae 100644
--- a/include/optutils.h
+++ b/include/optutils.h
@@ -6,6 +6,56 @@
 #include "c.h"
 #include "nls.h"
 
+enum {
+	arg_in_use = 0,
+	arg_hide,
+	arg_deprecate,
+	arg_removed
+};
+
+struct option_arguments {
+	int id;
+	char *name;
+	int status;
+};
+
+static inline int parse_arg_name(const struct option_arguments *valid_args,
+				 const char *arg)
+{
+	size_t i;
+
+	for (i = 0; valid_args[i].name; i++) {
+		if (strcmp(valid_args[i].name, arg) != 0)
+			continue;
+		switch (valid_args[i].status) {
+		case arg_in_use:
+		case arg_hide:
+			return valid_args[i].id;
+		case arg_deprecate:
+			warnx(_("'%s' is deprecated"), arg);
+			return valid_args[i].id;
+		case arg_removed:
+			warnx(_("'%s' is removed"), arg);
+			return -EINVAL;
+		default:
+			abort();
+		}
+	}
+	return -EINVAL;
+}
+
+static inline void print_valid_args(const struct option_arguments *valid_args,
+				    const char *message, FILE *out)
+{
+	size_t i;
+
+	fprintf(out, message);
+	for (i = 0; valid_args[i].name; i++)
+		if (valid_args[i].status == arg_in_use)
+			fprintf(out, " %s", valid_args[i].name);
+	fputc('\n', out);
+}
+
 static inline const char *option_to_longopt(int c, const struct option *opts)
 {
 	const struct option *o;
diff --git a/misc-utils/blkid.c b/misc-utils/blkid.c
index e0be5120c..5e3f58838 100644
--- a/misc-utils/blkid.c
+++ b/misc-utils/blkid.c
@@ -19,36 +19,6 @@
 #include <errno.h>
 #include <getopt.h>
 
-enum {
-	OUTPUT_FULL = 0,
-	OUTPUT_VALUE_ONLY,
-	OUTPUT_DEVICE_ONLY,
-	OUTPUT_PRETTY_LIST,
-	OUTPUT_UDEV_LIST,
-	OUTPUT_EXPORT_LIST
-};
-
-enum {
-	opt_in_use = 0,
-	opt_hide,
-	opt_deprecate,
-	opt_removed
-};
-
-struct option_arguments {
-	char *name;
-	int status;
-};
-
-static struct option_arguments output_formats[] = {
-	[OUTPUT_FULL]		= { "full", opt_in_use },
-	[OUTPUT_VALUE_ONLY]	= { "value", opt_in_use },
-	[OUTPUT_DEVICE_ONLY]	= { "device", opt_in_use },
-	[OUTPUT_PRETTY_LIST]	= { "list", opt_hide },		/* to be deprecated */
-	[OUTPUT_UDEV_LIST]	= { "udev", opt_hide },		/* to be deprecated */
-	[OUTPUT_EXPORT_LIST]	= { "export", opt_in_use },
-};
-
 #define LOWPROBE_TOPOLOGY	(1 << 1)
 #define LOWPROBE_SUPERBLOCKS	(1 << 2)
 
@@ -71,6 +41,25 @@ static struct option_arguments output_formats[] = {
 #include "ttyutils.h"
 #include "xalloc.h"
 
+enum {
+	OUTPUT_FULL = 0,
+	OUTPUT_VALUE_ONLY,
+	OUTPUT_DEVICE_ONLY,
+	OUTPUT_PRETTY_LIST,
+	OUTPUT_UDEV_LIST,
+	OUTPUT_EXPORT_LIST
+};
+
+static struct option_arguments output_formats[] = {
+	{ OUTPUT_FULL,  "full", arg_in_use },
+	{ OUTPUT_VALUE_ONLY, "value", arg_in_use },
+	{ OUTPUT_DEVICE_ONLY, "device", arg_in_use },
+	{ OUTPUT_PRETTY_LIST, "list", arg_hide },	/* to be deprecated */
+	{ OUTPUT_UDEV_LIST, "udev", arg_hide },		/* to be deprecated */
+	{ OUTPUT_EXPORT_LIST, "export", arg_in_use },
+	{ 0, NULL, 0 }
+};
+
 static int raw_chars;
 
 static void print_version(FILE *out)
@@ -98,8 +87,8 @@ static void usage(int error)
 	fputs(_(	" -d          don't encode non-printing characters\n"), out);
 	fputs(_(	" -h          print this usage message and exit\n"), out);
 	fputs(_(	" -g          garbage collect the blkid cache\n"), out);
-	fputs(_(	" -o <format> output format; can be one of:\n"
-			"               value, device, export or full; (default: full)\n"), out);
+	fputs(_(	" -o <format> output format; can be one of:\n"), out);
+	print_valid_args(output_formats, "              ", out);
 	fputs(_(	" -k          list all known filesystems/RAIDs and exit\n"), out);
 	fputs(_(	" -s <tag>    show specified tag(s) (default show all tags)\n"), out);
 	fputs(_(	" -t <token>  find device with a specific token (NAME=value pair)\n"), out);
@@ -121,36 +110,6 @@ static void usage(int error)
 	exit(error);
 }
 
-static int parse_format_name(const char *arg)
-{
-	size_t i;
-
-	for (i = 0; i < ARRAY_SIZE(output_formats); i++) {
-		if (strcmp(output_formats[i].name, arg) != 0)
-			continue;
-		switch (output_formats[i].status) {
-		case opt_in_use:
-		case opt_hide:
-			return i;
-		case opt_deprecate:
-			warnx(_("'%s' is deprecated"), arg);
-			return i;
-		case opt_removed:
-			warnx(_("'%s' is removed"), arg);
-			return -EINVAL;
-		default:
-			abort();
-		}
-	}
-	warnx(_("invalid argument: '%s'"), arg);
-	fprintf(stderr, _("recommended arguments to use are:"));
-	for (i = 0; i < ARRAY_SIZE(output_formats); i++)
-		if (output_formats[i].status == opt_in_use)
-			fprintf(stderr, " %s", output_formats[i].name);
-	fputc('\n', stderr);
-	return -EINVAL;
-}
-
 /*
  * This function does "safe" printing.  It will convert non-printable
  * ASCII characters using '^' and M- notation.
@@ -770,9 +729,9 @@ int main(int argc, char **argv)
 			exit(EXIT_SUCCESS);
 		}
 		case 'o':
-			output_format = parse_format_name(optarg);
+			output_format = parse_arg_name(output_formats, optarg);
 			if (output_format < 0)
-				exit(BLKID_EXIT_OTHER);
+				errx(BLKID_EXIT_OTHER, _("invalid argument: %s"), optarg);
 			break;
 		case 'O':
 			offset = strtosize_or_err(optarg, _("invalid offset argument"));

Re: [PATCH 07/13] blkid: define output names only once

[J William Piggott]

On 03/10/2017 09:48 AM, Sami Kerola wrote:
> On Fri, 10 Mar 2017, Sami Kerola wrote:
>> Notably I did not move list of recommended arguments to usage(). To do 
>> that well will require inu_use_arguments() function, that can be used 
>> where ever needed. But lets leave this as-is, and do these tidy 
>> adjustments if there it's seen these option argument functions should be 
>> generalized.
> 
> Hi again,
> 
> Having day off and idea to generalize this did not go away from my mind, 

 I'm glad that I'm not the only one with this trouble :)

> so I decided to give it a shot. I'm not sure how many utils might need 
> this, but that is hardly a reason against or in favour to include this. 
> Better question is if this is the way argument lists should be managed 
> when they are deprecated.

Seems correct to me on both accounts; with a few caveats:

The following are based my interpretation of the patch. I haven't
actually tested it. So I may have misread something.

* Is arg_removed needed? I think a 'removed' argument should be handled
  the same as an 'invalid' argument. Saying it was removed doesn't seem
  helpful and could be confusing.

* blkid: 'udev' is deprecated
  Seems ambiguous to me. Perhaps follow your default message style of:
  "invalid argument: udev". Like "deprecated argument: udev".

* What about this scenario:
  mycommand -a udev -b udev -c udev
   mycommand: 'udev' is deprecated
   mycommand: 'udev' is removed

Which is deprecated, which is removed, and which is in_use?
Is it the calling code's job to add the option name somehow?
Do we add 'char *opt_name' to 'struct option_arguments'?
Do we whistle a happy song and pretend this scenario cannot happen? :)


> +	int id;
> +	char *name;
> +	int status;
> +}
> 
> The below change can also be found from
>   git://github.com/kerolasa/lelux-utiliteetit.git 2017wk09
> 
> --->8----
> From: Sami Kerola <kerolasa@iki.fi>
> Date: Fri, 10 Mar 2017 14:06:10 +0000
> Subject: [PATCH] include/optutils: add option argument parsing and printing functions
> 
> Functions parse_arg_name() and print_valid_args() add methods that make
> option argument life cycle management easy.
> 
> Based-on-feedback-from: J William Piggott <elseifthen@gmx.com>
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  include/optutils.h | 50 +++++++++++++++++++++++++++++++
>  misc-utils/blkid.c | 87 
> +++++++++++++++---------------------------------------
>  2 files changed, 73 insertions(+), 64 deletions(-)
> 
> diff --git a/include/optutils.h b/include/optutils.h
> index 325cb8812..62ccc90ae 100644
> --- a/include/optutils.h
> +++ b/include/optutils.h
> @@ -6,6 +6,56 @@
>  #include "c.h"
>  #include "nls.h"
>  
> +enum {
> +	arg_in_use = 0,
> +	arg_hide,
> +	arg_deprecate,
> +	arg_removed
> +};
> +
> +struct option_arguments {
> +	int id;
> +	char *name;
> +	int status;
> +};
> +
> +static inline int parse_arg_name(const struct option_arguments *valid_args,
> +				 const char *arg)
> +{
> +	size_t i;
> +
> +	for (i = 0; valid_args[i].name; i++) {
> +		if (strcmp(valid_args[i].name, arg) != 0)
> +			continue;
> +		switch (valid_args[i].status) {
> +		case arg_in_use:
> +		case arg_hide:
> +			return valid_args[i].id;
> +		case arg_deprecate:
> +			warnx(_("'%s' is deprecated"), arg);
> +			return valid_args[i].id;
> +		case arg_removed:
> +			warnx(_("'%s' is removed"), arg);
> +			return -EINVAL;
> +		default:
> +			abort();
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static inline void print_valid_args(const struct option_arguments *valid_args,
> +				    const char *message, FILE *out)
> +{
> +	size_t i;
> +
> +	fprintf(out, message);
> +	for (i = 0; valid_args[i].name; i++)
> +		if (valid_args[i].status == arg_in_use)
> +			fprintf(out, " %s", valid_args[i].name);
> +	fputc('\n', out);
> +}
> +
>  static inline const char *option_to_longopt(int c, const struct option *opts)
>  {
>  	const struct option *o;
> diff --git a/misc-utils/blkid.c b/misc-utils/blkid.c
> index e0be5120c..5e3f58838 100644
> --- a/misc-utils/blkid.c
> +++ b/misc-utils/blkid.c
> @@ -19,36 +19,6 @@
>  #include <errno.h>
>  #include <getopt.h>
>  
> -enum {
> -	OUTPUT_FULL = 0,
> -	OUTPUT_VALUE_ONLY,
> -	OUTPUT_DEVICE_ONLY,
> -	OUTPUT_PRETTY_LIST,
> -	OUTPUT_UDEV_LIST,
> -	OUTPUT_EXPORT_LIST
> -};
> -
> -enum {
> -	opt_in_use = 0,
> -	opt_hide,
> -	opt_deprecate,
> -	opt_removed
> -};
> -
> -struct option_arguments {
> -	char *name;
> -	int status;
> -};
> -
> -static struct option_arguments output_formats[] = {
> -	[OUTPUT_FULL]		= { "full", opt_in_use },
> -	[OUTPUT_VALUE_ONLY]	= { "value", opt_in_use },
> -	[OUTPUT_DEVICE_ONLY]	= { "device", opt_in_use },
> -	[OUTPUT_PRETTY_LIST]	= { "list", opt_hide },		/* to be deprecated */
> -	[OUTPUT_UDEV_LIST]	= { "udev", opt_hide },		/* to be deprecated */
> -	[OUTPUT_EXPORT_LIST]	= { "export", opt_in_use },
> -};
> -
>  #define LOWPROBE_TOPOLOGY	(1 << 1)
>  #define LOWPROBE_SUPERBLOCKS	(1 << 2)
>  
> @@ -71,6 +41,25 @@ static struct option_arguments output_formats[] = {
>  #include "ttyutils.h"
>  #include "xalloc.h"
>  
> +enum {
> +	OUTPUT_FULL = 0,
> +	OUTPUT_VALUE_ONLY,
> +	OUTPUT_DEVICE_ONLY,
> +	OUTPUT_PRETTY_LIST,
> +	OUTPUT_UDEV_LIST,
> +	OUTPUT_EXPORT_LIST
> +};
> +
> +static struct option_arguments output_formats[] = {
> +	{ OUTPUT_FULL,  "full", arg_in_use },
> +	{ OUTPUT_VALUE_ONLY, "value", arg_in_use },
> +	{ OUTPUT_DEVICE_ONLY, "device", arg_in_use },
> +	{ OUTPUT_PRETTY_LIST, "list", arg_hide },	/* to be deprecated */
> +	{ OUTPUT_UDEV_LIST, "udev", arg_hide },		/* to be deprecated */
> +	{ OUTPUT_EXPORT_LIST, "export", arg_in_use },
> +	{ 0, NULL, 0 }
> +};
> +
>  static int raw_chars;
>  
>  static void print_version(FILE *out)
> @@ -98,8 +87,8 @@ static void usage(int error)
>  	fputs(_(	" -d          don't encode non-printing characters\n"), out);
>  	fputs(_(	" -h          print this usage message and exit\n"), out);
>  	fputs(_(	" -g          garbage collect the blkid cache\n"), out);
> -	fputs(_(	" -o <format> output format; can be one of:\n"
> -			"               value, device, export or full; (default: full)\n"), out);
> +	fputs(_(	" -o <format> output format; can be one of:\n"), out);
> +	print_valid_args(output_formats, "              ", out);
>  	fputs(_(	" -k          list all known filesystems/RAIDs and exit\n"), out);
>  	fputs(_(	" -s <tag>    show specified tag(s) (default show all tags)\n"), out);
>  	fputs(_(	" -t <token>  find device with a specific token (NAME=value pair)\n"), out);
> @@ -121,36 +110,6 @@ static void usage(int error)
>  	exit(error);
>  }
>  
> -static int parse_format_name(const char *arg)
> -{
> -	size_t i;
> -
> -	for (i = 0; i < ARRAY_SIZE(output_formats); i++) {
> -		if (strcmp(output_formats[i].name, arg) != 0)
> -			continue;
> -		switch (output_formats[i].status) {
> -		case opt_in_use:
> -		case opt_hide:
> -			return i;
> -		case opt_deprecate:
> -			warnx(_("'%s' is deprecated"), arg);
> -			return i;
> -		case opt_removed:
> -			warnx(_("'%s' is removed"), arg);
> -			return -EINVAL;
> -		default:
> -			abort();
> -		}
> -	}
> -	warnx(_("invalid argument: '%s'"), arg);
> -	fprintf(stderr, _("recommended arguments to use are:"));
> -	for (i = 0; i < ARRAY_SIZE(output_formats); i++)
> -		if (output_formats[i].status == opt_in_use)
> -			fprintf(stderr, " %s", output_formats[i].name);
> -	fputc('\n', stderr);
> -	return -EINVAL;
> -}
> -
>  /*
>   * This function does "safe" printing.  It will convert non-printable
>   * ASCII characters using '^' and M- notation.
> @@ -770,9 +729,9 @@ int main(int argc, char **argv)
>  			exit(EXIT_SUCCESS);
>  		}
>  		case 'o':
> -			output_format = parse_format_name(optarg);
> +			output_format = parse_arg_name(output_formats, optarg);
>  			if (output_format < 0)
> -				exit(BLKID_EXIT_OTHER);
> +				errx(BLKID_EXIT_OTHER, _("invalid argument: %s"), optarg);
>  			break;
>  		case 'O':
>  			offset = strtosize_or_err(optarg, _("invalid offset argument"));
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 07/13] blkid: define output names only once

[Karel Zak]

On Fri, Mar 10, 2017 at 02:48:24PM +0000, Sami Kerola wrote:
> Having day off and idea to generalize this did not go away from my mind, 
> so I decided to give it a shot. I'm not sure how many utils might need 

 The problem I see is that you force code writers to use your
 option_argument although the option argument may be part of the
 bigger picture. This is reason why for example string_to_idarray()
 string_to_bitarray() parses use name2id() callback.

 Anyway, we have probably more places where a simple string to id
 conversion is used. My recommendation is to add to lib/strutils.c
 string_to_idx() and use array of strings and return array index:

static int string_to_idx(const char **ary, size_t n, const char *name)
{
	size_t i;

	for (i = 0; i < n; i++) {
		if (strcmp(ary[i], name) == 0)
			return i;
	}
	return -1;
}

 such function you can use on many many places, not only for options
 arguments parsing.

> +static inline int parse_arg_name(const struct option_arguments *valid_args,
> +				 const char *arg)
> +{
> +	size_t i;
> +
> +	for (i = 0; valid_args[i].name; i++) {
> +		if (strcmp(valid_args[i].name, arg) != 0)
> +			continue;
> +		switch (valid_args[i].status) {
> +		case arg_in_use:
> +		case arg_hide:
> +			return valid_args[i].id;
> +		case arg_deprecate:
> +			warnx(_("'%s' is deprecated"), arg);
> +			return valid_args[i].id;

 The warning is unacceptable. Let's imagine you use old udev and you
 call blkid in your udev rules...

 It's always better to minimize number of messages for tools executed
 from scripts.

 All the valid_args->status seems like over-engineering ;-)

    Karel

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

[PATCH 08/13] blkid: improve coding style

[Sami Kerola]

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

diff --git a/misc-utils/blkid.c b/misc-utils/blkid.c
index 887a80083..493251d37 100644
--- a/misc-utils/blkid.c
+++ b/misc-utils/blkid.c
@@ -186,12 +186,12 @@ static void pretty_print_line(const char *device, const char *fs_type,
 		w = term_width / 10;
 		if (w > 8)
 			w = 8;
-		term_width -= 2*w;
+		term_width -= 2 * w;
 		label_len += w;
 		fs_type_len += w;
-		w = term_width/2;
+		w = term_width / 2;
 		device_len += w;
-		mtpt_len +=w;
+		mtpt_len += w;
 	}
 
 	len = pretty_print_word(device, device_len, 0, 1);
@@ -215,7 +215,7 @@ static void pretty_print_dev(blkid_dev dev)
 	if (dev == NULL) {
 		pretty_print_line("device", "fs_type", "label",
 				  "mount point", "UUID");
-		for (len=get_terminal_width(0)-1; len > 0; len--)
+		for (len = get_terminal_width(0) - 1; len > 0; len--)
 			fputc('-', stdout);
 		fputc('\n', stdout);
 		return;
@@ -265,8 +265,8 @@ static void print_udev_format(const char *name, const char *value)
 		printf("ID_FS_%s=%s\n", name, enc);
 
 	} else if (!strcmp(name, "UUID") ||
-		 !strcmp(name, "LABEL") ||
-		 !strcmp(name, "UUID_SUB")) {
+		   !strcmp(name, "LABEL") ||
+		   !strcmp(name, "UUID_SUB")) {
 
 		blkid_safe_string(value, safe, sizeof(safe));
 		printf("ID_FS_%s=%s\n", name, safe);
@@ -281,7 +281,7 @@ static void print_udev_format(const char *name, const char *value)
 		printf("ID_PART_TABLE_TYPE=%s\n", value);
 
 	} else if (!strcmp(name, "PART_ENTRY_NAME") ||
-		  !strcmp(name, "PART_ENTRY_TYPE")) {
+		   !strcmp(name, "PART_ENTRY_TYPE")) {
 
 		blkid_encode_string(value, enc, sizeof(enc));
 		printf("ID_%s=%s\n", name, enc);
@@ -289,11 +289,11 @@ static void print_udev_format(const char *name, const char *value)
 	} else if (!strncmp(name, "PART_ENTRY_", 11))
 		printf("ID_%s=%s\n", name, value);
 
-	else if (namelen >= 15 && (
-		   !strcmp(name + (namelen - 12), "_SECTOR_SIZE") ||
-		   !strcmp(name + (namelen - 8), "_IO_SIZE") ||
-		   !strcmp(name, "ALIGNMENT_OFFSET")))
-			printf("ID_IOLIMIT_%s=%s\n", name, value);
+	else if (namelen >= 15
+		 && (!strcmp(name + (namelen - 12), "_SECTOR_SIZE")
+		     || !strcmp(name + (namelen - 8), "_IO_SIZE")
+		     || !strcmp(name, "ALIGNMENT_OFFSET")))
+		printf("ID_IOLIMIT_%s=%s\n", name, value);
 	else
 		printf("ID_FS_%s=%s\n", name, value);
 }
@@ -382,7 +382,6 @@ static void print_tags(blkid_dev dev, char *show[], int output)
 	}
 }
 
-
 static int append_str(char **res, size_t *sz, const char *a, const char *b)
 {
 	char *str = *res;
@@ -510,7 +509,7 @@ static int lowprobe_device(blkid_probe pr, const char *devname,
 	int rc = 0;
 	static int first = 1;
 
-	fd = open(devname, O_RDONLY|O_CLOEXEC);
+	fd = open(devname, O_RDONLY | O_CLOEXEC);
 	if (fd < 0) {
 		warn(_("error: %s"), devname);
 		return BLKID_EXIT_NOTFOUND;
@@ -586,7 +585,7 @@ static int list_to_usage(const char *list, int *flag)
 	}
 	if (!p || !*p)
 		goto err;
-	while(p) {
+	while (p) {
 		word = p;
 		p = strchr(p, ',');
 		if (p)
@@ -625,14 +624,16 @@ static char **list_to_types(const char *list, int *flag)
 		warnx(_("error: -u <list> argument is empty"));
 		goto err;
 	}
-	for (i = 1; p && (p = strchr(p, ',')); i++, p++);
+	for (i = 1; p && (p = strchr(p, ',')); i++, p++)
+		/* do nothing */ ;
 
 	res = xcalloc(i + 1, sizeof(char *));
 	p = *flag & BLKID_FLTR_NOTIN ? list + 2 : list;
 	i = 0;
 
-	while(p) {
+	while (p) {
 		const char *word = p;
+
 		p = strchr(p, ',');
 		res[i++] = p ? xstrndup(word, p - word) : xstrdup(word);
 		if (p)
@@ -796,7 +797,6 @@ int main(int argc, char **argv)
 		}
 	}
 
-
 	/* The rest of the args are device names */
 	if (optind < argc) {
 		devices = xcalloc(argc - optind, sizeof(char *));
-- 
2.12.0

[PATCH 09/13] include: fix compiler warning

[Sami Kerola]

./include/optutils.h:12:18: warning: null pointer dereference [-Wnull-dereference]
  for (o = opts; o->name; o++)
                 ~^~~~~~
In file included from libfdisk/src/dos.c:12:0:
./include/pt-mbr.h:25:47: warning: potential null pointer dereference [-Wnull-dereference]
  return p[0] | (p[1] << 8) | (p[2] << 16) | (p[3] << 24);
         ~^~~

Well these should be impossible, so add assert() to catch possible bugs.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/optutils.h | 3 +++
 include/pt-mbr.h   | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/optutils.h b/include/optutils.h
index 99ad7e4b4..325cb8812 100644
--- a/include/optutils.h
+++ b/include/optutils.h
@@ -1,6 +1,8 @@
 #ifndef UTIL_LINUX_OPTUTILS_H
 #define UTIL_LINUX_OPTUTILS_H
 
+#include <assert.h>
+
 #include "c.h"
 #include "nls.h"
 
@@ -8,6 +10,7 @@ static inline const char *option_to_longopt(int c, const struct option *opts)
 {
 	const struct option *o;
 
+	assert(!(opts == NULL));
 	for (o = opts; o->name; o++)
 		if (o->val == c)
 			return o->name;
diff --git a/include/pt-mbr.h b/include/pt-mbr.h
index 90be95b1f..177cc7474 100644
--- a/include/pt-mbr.h
+++ b/include/pt-mbr.h
@@ -1,6 +1,8 @@
 #ifndef UTIL_LINUX_PT_MBR_H
 #define UTIL_LINUX_PT_MBR_H
 
+#include <assert.h>
+
 struct dos_partition {
 	unsigned char boot_ind;		/* 0x80 - active */
 	unsigned char bh, bs, bc;	/* begin CHS */
@@ -27,6 +29,7 @@ static inline unsigned int __dos_assemble_4le(const unsigned char *p)
 
 static inline void __dos_store_4le(unsigned char *p, unsigned int val)
 {
+	assert(!(p == NULL));
 	p[0] = (val & 0xff);
 	p[1] = ((val >> 8) & 0xff);
 	p[2] = ((val >> 16) & 0xff);
-- 
2.12.0

[PATCH 10/13] libfdisk: dos: fix potential null pointer dereferences

[Sami Kerola]

Lots of the following warnings fixed, where all originated to self_pte()
that can return NULL.

libfdisk/src/dos.c:1850:25: warning: potential null pointer dereference
[-Wnull-dereference]
   struct dos_partition *p = pe->pt_entry;

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 libfdisk/src/dos.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 6 deletions(-)

diff --git a/libfdisk/src/dos.c b/libfdisk/src/dos.c
index 11946b38c..8b9ff464c 100644
--- a/libfdisk/src/dos.c
+++ b/libfdisk/src/dos.c
@@ -311,6 +311,8 @@ static void dos_init(struct fdisk_context *cxt)
 	for (i = 0; i < 4; i++) {
 		struct pte *pe = self_pte(cxt, i);
 
+		if (pe == NULL)
+			return;
 		pe->pt_entry = mbr_get_partition(cxt->firstsector, i);
 		pe->ex_entry = NULL;
 		pe->offset = 0;
@@ -502,6 +504,8 @@ static void read_extended(struct fdisk_context *cxt, size_t ext)
 
 	l->ext_index = ext;
 	pex = self_pte(cxt, ext);
+	if (pex == NULL)
+		return;
 	pex->ex_entry = pex->pt_entry;
 
 	p = pex->pt_entry;
@@ -515,12 +519,16 @@ static void read_extended(struct fdisk_context *cxt, size_t ext)
 	while (IS_EXTENDED (p->sys_ind)) {
 		pe = self_pte(cxt, cxt->label->nparts_max);
 
+		if (pe == NULL)
+			return;
 		if (cxt->label->nparts_max >= MAXIMUM_PARTS) {
 			/* This is not a Linux restriction, but
 			   this program uses arrays of size MAXIMUM_PARTS.
 			   Do not try to `improve' this test. */
 			struct pte *pre = self_pte(cxt,
 						cxt->label->nparts_max - 1);
+			if (pre == NULL)
+				return;
 			fdisk_warnx(cxt,
 			_("Omitting partitions after #%zu. They will be deleted "
 			  "if you save this partition table."),
@@ -596,6 +604,8 @@ static void read_extended(struct fdisk_context *cxt, size_t ext)
 
 	/* remove last empty EBR */
 	pe = self_pte(cxt, cxt->label->nparts_max - 1);
+	if (pe == NULL)
+		return;
 	if (is_cleared_partition(pe->ex_entry) &&
 	    is_cleared_partition(pe->pt_entry)) {
 		DBG(LABEL, ul_debug("DOS: EBR[offset=%ju]: empty, remove", (uintmax_t) pe->offset));
@@ -610,6 +620,8 @@ static void read_extended(struct fdisk_context *cxt, size_t ext)
 	for (i = 4; i < cxt->label->nparts_max; i++) {
 		p = self_partition(cxt, i);
 
+		if (p == NULL)
+			continue;
 		if (!dos_partition_get_size(p) &&
 		    (cxt->label->nparts_max > 5 || q->sys_ind)) {
 			fdisk_info(cxt, _("omitting empty partition (%zu)"), i+1);
@@ -798,6 +810,8 @@ static int dos_probe_label(struct fdisk_context *cxt)
 	for (i = 0; i < 4; i++) {
 		struct pte *pe = self_pte(cxt, i);
 
+		if (pe == NULL)
+			return -1;
 		if (is_used_partition(pe->pt_entry))
 			cxt->label->nparts_cur++;
 
@@ -884,6 +898,8 @@ static fdisk_sector_t get_unused_start(struct fdisk_context *cxt,
 		fdisk_sector_t lastplusoff;
 		struct pte *pe = self_pte(cxt, i);
 
+		if (pe == NULL)
+			return UINT64_MAX;
 		if (start == pe->offset)
 			start += cxt->first_lba;
 		lastplusoff = last[i] + ((part_n < 4) ? 0 : cxt->first_lba);
@@ -1007,6 +1023,8 @@ static fdisk_sector_t get_unused_last(struct fdisk_context *cxt, size_t n,
 	for (i = 0; i < cxt->label->nparts_max; i++) {
 		struct pte *pe = self_pte(cxt, i);
 
+		if (pe == NULL)
+			return 0;
 		if (start < pe->offset && limit >= pe->offset)
 			limit = pe->offset - 1;
 		if (start < first[i] && limit >= first[i])
@@ -1087,6 +1105,8 @@ static int add_partition(struct fdisk_context *cxt, size_t n,
 
 		temp = start;
 		dflt = start = get_unused_start(cxt, n, start, first, last);
+		if (start == UINT64_MAX)
+			continue;
 
 		if (n >= 4 && pa && fdisk_partition_has_start(pa) && cxt->script
 		    && cxt->first_lba > 1
@@ -1133,6 +1153,8 @@ static int add_partition(struct fdisk_context *cxt, size_t n,
 		/* The second (and another) EBR */
 		struct pte *pe = self_pte(cxt, n);
 
+		if (pe == NULL)
+			return -EINVAL;
 		pe->offset = start - cxt->first_lba;
 		if (pe->offset == l->ext_offset) { /* must be corrected */
 			pe->offset++;
@@ -1235,6 +1257,11 @@ static int add_partition(struct fdisk_context *cxt, size_t n,
 	set_partition(cxt, n, 0, start, stop, sys, fdisk_partition_is_bootable(pa));
 	if (n > 4) {
 		struct pte *pe = self_pte(cxt, n);
+
+		if (pe == NULL) {
+			rc = 1;
+			goto done;
+		}
 		set_partition(cxt, n - 1, 1, pe->offset, stop,
 					MBR_DOS_EXTENDED_PARTITION, 0);
 	}
@@ -1251,6 +1278,10 @@ static int add_partition(struct fdisk_context *cxt, size_t n,
 	if (IS_EXTENDED(sys)) {
 		struct pte *pen = self_pte(cxt, n);
 
+		if (pen == NULL) {
+			rc = 1;
+			goto done;
+		}
 		l->ext_index = n;
 		l->ext_offset = start;
 		pen->ex_entry = p;
@@ -1278,6 +1309,8 @@ static int add_logical(struct fdisk_context *cxt,
 	DBG(LABEL, ul_debug("DOS: nparts max: %zu", cxt->label->nparts_max));
 	pe = self_pte(cxt, cxt->label->nparts_max);
 
+	if (pe == NULL)
+		return -1;
 	if (!pe->sectorbuffer) {
 		pe->sectorbuffer = calloc(1, cxt->sector_size);
 		if (!pe->sectorbuffer)
@@ -1476,6 +1509,8 @@ static int dos_verify_disklabel(struct fdisk_context *cxt)
 			total++;
 			p = self_partition(cxt, i);
 
+			if (p == NULL)
+				continue;
 			if (!p->sys_ind) {
 				if (i != 4 || i + 1 < cxt->label->nparts_max)
 					fdisk_warnx(cxt,
@@ -1759,7 +1794,8 @@ static int dos_write_disklabel(struct fdisk_context *cxt)
 	if (!mbr_changed) {
 		for (i = 0; i < 4; i++) {
 			struct pte *pe = self_pte(cxt, i);
-			if (pe->changed)
+
+			if (pe && pe->changed)
 				mbr_changed = 1;
 		}
 	}
@@ -1788,7 +1824,7 @@ static int dos_write_disklabel(struct fdisk_context *cxt)
 	for (i = 4; i < cxt->label->nparts_max; i++) {
 		struct pte *pe = self_pte(cxt, i);
 
-		if (!pe->changed || !pe->offset || !pe->sectorbuffer)
+		if (!pe || !pe->changed || !pe->offset || !pe->sectorbuffer)
 			continue;
 
 		mbr_set_magic(pe->sectorbuffer);
@@ -1847,7 +1883,7 @@ static int wrong_p_order(struct fdisk_context *cxt, size_t *prev)
 	for (i = 0 ; i < cxt->label->nparts_max; i++) {
 
 		struct pte *pe = self_pte(cxt, i);
-		struct dos_partition *p = pe->pt_entry;
+		struct dos_partition *p = pe ? pe->pt_entry : NULL;
 
 		if (i == 4) {
 			last_i = 4;
@@ -1913,6 +1949,8 @@ static int dos_get_partition(struct fdisk_context *cxt, size_t n,
 
 	lb = self_label(cxt);
 	pe = self_pte(cxt, n);
+	if (pe == NULL)
+		return -EINVAL;
 	p = pe->pt_entry;
 	pa->used = !is_cleared_partition(p);
 	if (!pa->used)
@@ -2058,6 +2096,8 @@ static void print_chain_of_logicals(struct fdisk_context *cxt)
 	for (i = 4; i < cxt->label->nparts_max; i++) {
 		struct pte *pe = self_pte(cxt, i);
 
+		if (pe == NULL)
+			return;
 		fprintf(stderr, "#%02zu EBR [%10ju], "
 			"data[start=%10ju (%10ju), size=%10ju], "
 			"link[start=%10ju (%10ju), size=%10ju]\n",
@@ -2140,9 +2180,12 @@ again:
 	for (i = 4; i < cxt->label->nparts_max - 1; i++) {
 		struct pte *cur = self_pte(cxt, i),
 			   *nxt = self_pte(cxt, i + 1);
+		fdisk_sector_t noff, ooff;
 
-		fdisk_sector_t noff = nxt->offset - l->ext_offset,
-			 ooff = dos_partition_get_start(cur->ex_entry);
+		if (cur == NULL || nxt == NULL)
+			continue;
+		noff = nxt->offset - l->ext_offset;
+		ooff = dos_partition_get_start(cur->ex_entry);
 
 		if (noff == ooff)
 			continue;
@@ -2182,6 +2225,8 @@ static int dos_reorder(struct fdisk_context *cxt)
 		struct dos_partition *pi, *pk, *pe, pbuf;
 		pei = self_pte(cxt, i);
 		pek = self_pte(cxt, k);
+		if (pei == NULL || pek == NULL)
+			return -1;
 
 		pe = pei->ex_entry;
 		pei->ex_entry = pek->ex_entry;
@@ -2218,6 +2263,10 @@ int fdisk_dos_move_begin(struct fdisk_context *cxt, size_t i)
 	assert(fdisk_is_label(cxt, DOS));
 
 	pe = self_pte(cxt, i);
+	if (pe == NULL) {
+		fdisk_warnx(cxt, _("Partition %zu: no pte_label."), i + 1);
+		return 0;
+	}
 	p = pe->pt_entry;
 
 	if (!is_used_partition(p) || IS_EXTENDED (p->sys_ind)) {
@@ -2236,7 +2285,7 @@ int fdisk_dos_move_begin(struct fdisk_context *cxt, size_t i)
 	for (x = 0; x < cxt->label->nparts_max; x++) {
 		unsigned int end;
 		struct pte *prev_pe = self_pte(cxt, x);
-		struct dos_partition *prev_p = prev_pe->pt_entry;
+		struct dos_partition *prev_p = prev_pe ? prev_pe->pt_entry : NULL;
 
 		if (!prev_p)
 			continue;
-- 
2.12.0

[PATCH 11/13] libfdisk: fix potentially null

[Sami Kerola]

1248:3: note: in expansion of macro 'FDISK_INIT_UNDEF'
   FDISK_INIT_UNDEF(xpa->size);
   ^~~~~~~~~~~~~~~~
libfdisk/src/partition.c:1247:15: warning: potential null pointer
dereference [-Wnull-dereference]
   xpa->resize = 0;
   ~~~~~~~~~~~~^~~
libfdisk/src/partition.c:1246:18: warning: potential null pointer
dereference [-Wnull-dereference]
   xpa->movestart = 0;
   ~~~~~~~~~~~~~~~^~~

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 libfdisk/src/partition.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libfdisk/src/partition.c b/libfdisk/src/partition.c
index 1e8c1eb32..adb4acfe4 100644
--- a/libfdisk/src/partition.c
+++ b/libfdisk/src/partition.c
@@ -1243,6 +1243,10 @@ int fdisk_set_partition(struct fdisk_context *cxt, size_t partno,
 
 	if (pa->resize || fdisk_partition_has_start(pa) || fdisk_partition_has_size(pa)) {
 		xpa = __copy_partition(pa);
+		if (xpa == NULL) {
+			rc = 1;
+			goto done;
+		}
 		xpa->movestart = 0;
 		xpa->resize = 0;
 		FDISK_INIT_UNDEF(xpa->size);
-- 
2.12.0

[PATCH 12/13] docs: improve boilerplate.c options example

[Sami Kerola]

Make usage() translations to contain complete option help in single
translation unit.  And make -z option example complete.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 Documentation/boilerplate.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/boilerplate.c b/Documentation/boilerplate.c
index b210cc363..7da9374d4 100644
--- a/Documentation/boilerplate.c
+++ b/Documentation/boilerplate.c
@@ -39,11 +39,11 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
 	fputs(_(" -r, --required <arg>    option requires an argument\n"), out);
 	fputs(_(" -z                      no long option\n"), out);
 	fputs(_("     --xyzzy             a long option only\n"), out);
-	fputs(_(" -e, --extremely-long-long-option\n"), out);
-	fputs(_("                         use next line for description when needed\n"), out);
-	fputs(_(" -l, --long-explanation  an example of very verbose, and chatty option\n"), out);
-	fputs(_("                           description on two, or multiple lines, where the\n"), out);
-	fputs(_("                           consecutive lines are intended by two spaces\n"), out);
+	fputs(_(" -e, --extremely-long-long-option\n"
+		"                         use next line for description when needed\n"), out);
+	fputs(_(" -l, --long-explanation  an example of very verbose, and chatty option\n"
+		"                           description on two, or multiple lines, where the\n"
+		"                           consecutive lines are intended by two spaces\n"), out);
 	fputs(_(" -f, --foobar            next option description resets indent\n"), out);
 	fputs(USAGE_SEPARATOR, out);
 	fputs(USAGE_HELP, out);
@@ -77,11 +77,12 @@ int main(int argc, char **argv)
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	while ((c = getopt_long(argc, argv, "nr:elfVh", longopts, NULL)) != -1)
+	while ((c = getopt_long(argc, argv, "nr:zelfVh", longopts, NULL)) != -1)
 		switch (c) {
 		case 'n':
 		case OPT_OPTIONAL:
 		case 'r':
+		case 'z':
 		case OPT_XYZZY:
 		case 'e':
 		case 'l':
-- 
2.12.0

[PATCH 13/13] lib/colors: assert rathen than overflow

[Sami Kerola]

lib/colors.c: In function 'colors_read_schemes':
lib/colors.c:412:7: warning: potential null pointer dereference [-Wnull-dereference]
  *out = '\0';
  ~~~~~^~~~~~

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 lib/colors.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/colors.c b/lib/colors.c
index 4b280fe27..baafe0a90 100644
--- a/lib/colors.c
+++ b/lib/colors.c
@@ -342,6 +342,7 @@ static char *colors_get_homedir(char *buf, size_t bufsz)
 static int cn_sequence(const char *str, char **seq)
 {
 	char *in, *out;
+	int len;
 
 	if (!str)
 		return -EINVAL;
@@ -357,7 +358,7 @@ static int cn_sequence(const char *str, char **seq)
 	}
 
 	/* convert xx;yy sequences to "\033[xx;yy" */
-	if (asprintf(seq, "\033[%sm", str) < 1)
+	if ((len = asprintf(seq, "\033[%sm", str)) < 1)
 		return -ENOMEM;
 
 	for (in = *seq, out = *seq; in && *in; in++) {
@@ -409,6 +410,7 @@ static int cn_sequence(const char *str, char **seq)
 		}
 		in++;
 	}
+	assert (len < (out - *seq));
 	*out = '\0';
 
 	return 0;
-- 
2.12.0

Re: [PATCH 13/13] lib/colors: assert rathen than overflow

[Karel Zak]

On Sun, Mar 05, 2017 at 08:52:32PM +0000, Sami Kerola wrote:
> lib/colors.c: In function 'colors_read_schemes':
> lib/colors.c:412:7: warning: potential null pointer dereference [-Wnull-dereference]
>   *out = '\0';
>   ~~~~~^~~~~~
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  lib/colors.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/colors.c b/lib/colors.c
> index 4b280fe27..baafe0a90 100644
> --- a/lib/colors.c
> +++ b/lib/colors.c
> @@ -342,6 +342,7 @@ static char *colors_get_homedir(char *buf, size_t bufsz)
>  static int cn_sequence(const char *str, char **seq)
>  {
>  	char *in, *out;
> +	int len;
>  
>  	if (!str)
>  		return -EINVAL;
> @@ -357,7 +358,7 @@ static int cn_sequence(const char *str, char **seq)
>  	}
>  
>  	/* convert xx;yy sequences to "\033[xx;yy" */
> -	if (asprintf(seq, "\033[%sm", str) < 1)
> +	if ((len = asprintf(seq, "\033[%sm", str)) < 1)
>  		return -ENOMEM;
>  
>  	for (in = *seq, out = *seq; in && *in; in++) {
> @@ -409,6 +410,7 @@ static int cn_sequence(const char *str, char **seq)
>  		}
>  		in++;
>  	}
> +	assert (len < (out - *seq));

This seems incorrect. Should be:

    assert ((out - *seq) <= len);

note that asprintf() returns size without terminating byte.

Hmm.. we have no test_colors used in our regression tests. Simple
test:

 $ echo 'mybell \a' > ~/.config/terminal-colors.d/foo.scheme
 $ TERMINAL_COLORS_DEBUG=all ./test_colors --name foo --color-scheme mybell

    Karel

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

Re: [PATCH 13/13] lib/colors: assert rathen than overflow

[Karel Zak]

On Mon, Mar 13, 2017 at 02:43:54PM +0100, Karel Zak wrote:
> > +	assert (len < (out - *seq));
> 
> This seems incorrect. Should be:
> 
>     assert ((out - *seq) <= len);

Merged with this line.

    Karel


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

Re: [PATCH 00/13] pull: warnings fixes and blkid translations

[Karel Zak]

On Sun, Mar 05, 2017 at 08:52:19PM +0000, Sami Kerola wrote:
> Sami Kerola (13):
>   misc: stop mixing declarations and code
>   libmount: fix use of preprocessor identifier
>   libblkid: avoid overflow when initializing array
>   libblkid: ensure uninitialized variable is not used [clang]
>   blkid: add curly braces to do-while statement
>   blkid: add messages to translations

Applied.

>   blkid: define output names only once
>   blkid: improve coding style

Not applied.

>   include: fix compiler warning

Applied.

>   libfdisk: dos: fix potential null pointer dereferences
>   libfdisk: fix potentially null

Applied a little different version.

>   docs: improve boilerplate.c options example

Applied.

>   lib/colors: assert rathen than overflow

Applied fixed version.

    Karel

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