Re: [RFC PATCH util-linux] text-utils: add bits command

On 2024-10-16 22:26:22+0200, Robin Jarry wrote:
> Add a new test utility to convert between bit masks in various formats.

"test" or "text" utility?

> This can be handy to avoid parsing affinity masks in one's head and/or
> to interact with the kernel in a more human friendly way.
> 
> This is a rewrite in C of the bits command from my linux-tools python
> package so that it can be more widely available.
> 
> Here is an example:
> 
>  ~# cat /sys/kernel/debug/tracing/tracing_cpumask
>  fffffff,ffffffff,ffffffff,ffffffff
>  ~# bits -l ,$(cat /sys/kernel/debug/tracing/tracing_cpumask)
>  0-128
>  ~# bits -g 58,59,120,123
>  9000000,00000000,0c000000,00000000
>  ~# bits -g 58,59 > /sys/kernel/debug/tracing/tracing_cpumask
>  ~# echo 1 > /sys/kernel/debug/tracing/tracing_on
> 
> Add man page and basic tests.
> 
> Link: https://git.sr.ht/~rjarry/linux-tools#bits
> Signed-off-by: Robin Jarry <[email protected]>
> ---
>  bash-completion/bits                  |  21 ++
>  meson.build                           |  11 ++
>  tests/commands.sh                     |   1 +
>  tests/expected/misc/bits              |   0
>  tests/expected/misc/bits-and          |   1 +
>  tests/expected/misc/bits-binary       |   1 +
>  tests/expected/misc/bits-default      |   1 +
>  tests/expected/misc/bits-grouped-mask |   1 +
>  tests/expected/misc/bits-list         |   1 +
>  tests/expected/misc/bits-mask         |   1 +
>  tests/expected/misc/bits-not          |   1 +
>  tests/expected/misc/bits-or           |   1 +
>  tests/expected/misc/bits-overflow     |   1 +
>  tests/expected/misc/bits-parse-mask   |   1 +
>  tests/expected/misc/bits-parse-range  |   1 +
>  tests/expected/misc/bits-stdin        |   1 +
>  tests/expected/misc/bits-xor          |   1 +
>  tests/ts/misc/bits                    |  82 ++++++++
>  text-utils/Makemodule.am              |   6 +
>  text-utils/bits.1.adoc                | 147 ++++++++++++++
>  text-utils/bits.c                     | 265 ++++++++++++++++++++++++++
>  text-utils/meson.build                |   4 +
>  22 files changed, 550 insertions(+)
>  create mode 100644 bash-completion/bits
>  create mode 100644 tests/expected/misc/bits
>  create mode 100644 tests/expected/misc/bits-and
>  create mode 100644 tests/expected/misc/bits-binary
>  create mode 100644 tests/expected/misc/bits-default
>  create mode 100644 tests/expected/misc/bits-grouped-mask
>  create mode 100644 tests/expected/misc/bits-list
>  create mode 100644 tests/expected/misc/bits-mask
>  create mode 100644 tests/expected/misc/bits-not
>  create mode 100644 tests/expected/misc/bits-or
>  create mode 100644 tests/expected/misc/bits-overflow
>  create mode 100644 tests/expected/misc/bits-parse-mask
>  create mode 100644 tests/expected/misc/bits-parse-range
>  create mode 100644 tests/expected/misc/bits-stdin
>  create mode 100644 tests/expected/misc/bits-xor
>  create mode 100755 tests/ts/misc/bits
>  create mode 100644 text-utils/bits.1.adoc
>  create mode 100644 text-utils/bits.c

...

> +++ b/tests/ts/misc/bits
> @@ -0,0 +1,82 @@
> +#!/bin/bash
> +
> +#
> +# Copyright (c) 2024 Robin Jarry
> +#
> +# This file is part of util-linux.
> +#
> +# This file is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This file is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.

New files should use SPDX tags.

> diff --git a/text-utils/bits.c b/text-utils/bits.c
> new file mode 100644
> index 000000000000..6dd0db81a5de
> --- /dev/null
> +++ b/text-utils/bits.c
> @@ -0,0 +1,265 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Copyright (c) 2024 Robin Jarry
> + *
> + * bits - convert bit masks from/to various formats
> + */
> +
> +#include <errno.h>
> +#include <getopt.h>
> +#include <sched.h>
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include "c.h"
> +#include "closestream.h"
> +#include "cpuset.h"
> +#include "nls.h"
> +#include "strutils.h"
> +#include "strv.h"
> +
> +typedef enum {
> +	OP_OR,
> +	OP_AND,
> +	OP_NOT,
> +	OP_XOR,
> +} bitvise_op_t;

"bitwise"?
Also avoid using the _t suffix as it's reserved.
"enum bitwise_op" seems clear enough.

> +
> +static int parse_mask_or_list(const char *cmdline_arg, cpu_set_t *all_bits)
> +{
> +	bitvise_op_t op = OP_OR;
> +	cpu_set_t bits, copy;

This seems very geared towards cpu masks.
What happens if a user wants to use it with very large non-CPU bitmasks,
as the tool is advertised as a generic utility?

Call the utility "cpumask"?

> +	char buf[BUFSIZ];

Only use BUFSIZ in cases where the actual value does not matter,
for example loops.
On musl BUFSIZ == 1024 and this could not be enough fairly soon.

> +	char *arg = buf;
> +
> +	/* copy to allow modifying the argument contents */
> +	strlcpy(buf, cmdline_arg, sizeof(buf));
> +	buf[sizeof(buf) - 1] = '\0';

You can just use xstrdup().

> +
> +	/* strip optional operator first */
> +	if (startswith(arg, "&")) {
> +		op = OP_AND;
> +		arg++;
> +	} else if (startswith(arg, "^")) {
> +		op = OP_XOR;
> +		arg++;
> +	} else if (startswith(arg, "~")) {
> +		op = OP_NOT;
> +		arg++;
> +	} else if (startswith(arg, "|")) {
> +		op = OP_OR;
> +		arg++;
> +	}

The whole op enum could also be replaced by the actual character,
this is all in a single function.

op = '&';

...

switch (op) {
case '&':
}

> +
> +	if (startswith(arg, ",") || startswith(arg, "0x")) {
> +		if (cpumask_parse(arg, &bits, sizeof(bits)) < 0)
> +			return -EINVAL;

As this is a commandline tool you can call errx() here to kill the
process and provide better error information.
It would also make the call sequence slightly more simple.

> +	} else {
> +		if (cpulist_parse(arg, &bits, sizeof(bits), 1) < 0)
> +			return -EINVAL;
> +	}
> +
> +	switch (op) {
> +	case OP_AND:
> +		copy = *all_bits;
> +		CPU_AND(all_bits, &copy, &bits);
> +		break;
> +	case OP_OR:
> +		copy = *all_bits;
> +		CPU_OR(all_bits, &copy, &bits);
> +		break;
> +	case OP_XOR:
> +		copy = *all_bits;
> +		CPU_XOR(all_bits, &copy, &bits);
> +		break;
> +	case OP_NOT:
> +		for (int i = 0; i < CPU_SETSIZE; i++) {
> +			if (CPU_ISSET(i, &bits))
> +				CPU_CLR(i, all_bits);
> +		}
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +typedef enum {
> +	MODE_BINARY,
> +	MODE_GROUPED_MASK,
> +	MODE_LIST,
> +	MODE_MASK,
> +} output_mode_t;
> +
> +static void print_bits(cpu_set_t *bits, output_mode_t mode)
> +{
> +	bool started = false;
> +	char buf[BUFSIZ];
> +	ssize_t n = 0;
> +
> +	buf[0] = '\0';

Is this necessary?

> +
> +	switch (mode) {
> +	case MODE_MASK:
> +		cpumask_create(buf, sizeof(buf), bits, sizeof(*bits));
> +
> +		/* strip leading zeroes */
> +		while (buf[n] == '0')
> +			n++;
> +		if (buf[n] == '\0')
> +			printf("0x0\n");
> +		else
> +			printf("0x%s\n", buf + n);
> +		break;
> +
> +	case MODE_GROUPED_MASK:
> +		cpumask_create(buf, sizeof(buf), bits, sizeof(*bits));
> +
> +		/* strip leading zeroes */
> +		while (buf[n] == '0')
> +			n++;
> +
> +		while (buf[n] != '\0') {
> +			if (started && (n % 8) == 0)
> +				printf(",");
> +			if (buf[n] != '0')
> +				started = true;
> +			printf("%c", buf[n]);
> +			n++;
> +		}
> +		printf("\n");
> +		break;
> +
> +	case MODE_BINARY:
> +		printf("0b");
> +		for (n = CPU_SETSIZE; n >= 0; n--) {
> +			if (started && ((n + 1) % 4) == 0)
> +				printf("_");
> +			if (CPU_ISSET(n, bits)) {
> +				started = true;
> +				printf("1");
> +			} else if (started) {
> +				printf("0");
> +			}
> +		}
> +		printf("\n");
> +		break;
> +
> +	case MODE_LIST:
> +		cpulist_create(buf, sizeof(buf), bits, sizeof(*bits));

This could use the return values of cpulist_create() and
cpumask_create().

> +		printf("%s\n", buf);
> +		break;
> +	}
> +
> +}
> +
> +static void __attribute__((__noreturn__)) usage(void)
> +{
> +	fputs(USAGE_HEADER, stdout);
> +	fprintf(stdout, _(" %s [options] [<mask_or_list>...]\n"), program_invocation_short_name);
> +
> +	fputs(USAGE_SEPARATOR, stdout);
> +	fputsln(_("Convert bit masks from/to various formats."), stdout);
> +
> +	fputs(USAGE_ARGUMENTS, stdout);
> +	fputsln(_(" <mask_or_list>      A set of bits specified as a hex mask value (e.g. 0xeec2)\n"
> +		"                     or as a comma-separated list of bit IDs.\n"
> +		"                     If not specified, arguments will be read from stdin."), stdout);
> +
> +	fputs(USAGE_OPTIONS, stdout);
> +	fprintf(stdout, USAGE_HELP_OPTIONS(21));
> +
> +	fputs(_("\nMode:\n"), stdout);
> +	fputsln(_(" -m, --mask          Print the combined args as a hex mask value (default).\n"), stdout);
> +	fputsln(_(" -g, --grouped-mask  Print the combined args as a hex mask value in 32bit\n"
> +			"                     comma separated groups."), stdout);
> +	fputsln(_(" -b, --binary        Print the combined args as a binary mask value."), stdout);
> +	fputsln(_(" -l, --list          Print the combined args as a compressed list of bit IDs."), stdout);
> +
> +	fputs(USAGE_SEPARATOR, stdout);
> +	fprintf(stdout, USAGE_MAN_TAIL("bits(1)"));
> +	exit(EXIT_SUCCESS);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	output_mode_t mode = MODE_MASK;
> +	char **stdin_lines = NULL;
> +	cpu_set_t bits;
> +	int c;
> +
> +	static const struct option longopts[] = {
> +		{ "version",      no_argument, NULL, 'V'},
> +		{ "help",         no_argument, NULL, 'h'},
> +		{ "mask",         no_argument, NULL, 'm'},
> +		{ "grouped-mask", no_argument, NULL, 'g'},
> +		{ "binary",       no_argument, NULL, 'b'},
> +		{ "list",         no_argument, NULL, 'l'},
> +		{ NULL, 0, NULL, 0 }
> +	};
> +
> +	setlocale(LC_ALL, "");
> +	bindtextdomain(PACKAGE, LOCALEDIR);
> +	textdomain(PACKAGE);
> +	close_stdout_atexit();
> +
> +	while ((c = getopt_long(argc, argv, "Vhmgbl", longopts, NULL)) != -1)
> +		switch (c) {
> +		case 'm':
> +			mode = MODE_MASK;
> +			break;
> +		case 'g':
> +			mode = MODE_GROUPED_MASK;
> +			break;
> +		case 'b':
> +			mode = MODE_BINARY;
> +			break;
> +		case 'l':
> +			mode = MODE_LIST;
> +			break;
> +		case 'V':
> +			print_version(EXIT_SUCCESS);
> +		case 'h':
> +			usage();
> +		default:
> +			errtryhelp(EXIT_FAILURE);
> +		}
> +
> +	argc -= optind;
> +	argv += optind;
> +	if (argc == 0) {
> +		/* no arguments provided, read lines from stdin */
> +		char buf[BUFSIZ];
> +
> +		while (fgets(buf, sizeof(buf), stdin)) {
> +			/* strip LF, CR, CRLF, LFCR */
> +			buf[strcspn(buf, "\r\n")] = '\0';

rtrim_whitespace()?

> +			strv_push(&stdin_lines, strdup(buf));
> +		}
> +
> +		argc = strv_length(stdin_lines);
> +		argv = stdin_lines;
> +	}
> +
> +	CPU_ZERO(&bits);
> +
> +	for (; argc > 0; argc--, argv++)
> +		if (parse_mask_or_list(*argv, &bits) < 0) {
> +			fprintf(stderr, _("error: invalid argument: %s\n"), *argv);
> +			errtryhelp(EXIT_FAILURE);
> +		}
> +
> +	strv_free(stdin_lines);

No need to free this here. The process will exit anyways.

> +
> +	print_bits(&bits, mode);
> +
> +	return EXIT_SUCCESS;
> +}