Oct 16, 2024 23:11:38 Robin Jarry <robin@xxxxxxxx>: > Hi Thomas, > > Thomas Weißschuh, Oct 16, 2024 at 23:00: >> 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? > > That was a typo, I meant "text" :) > >>> 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 <robin@xxxxxxxx> >>> --- >>> 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. > > Ack, will change in v2. > >>> 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. > > Yes, bitwise is the correct spelling. I will probably remove that in v2 as you suggested below. > >>> + >>> +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"? > > The default cpu_set_t size can hold up to 1024 bits (128 bytes). I assumed it was more than enough for common use cases. I added a basic test that checks that the program does not crash when specifying a too large mask. > I also didn't want to call that tool "*mask" because it can also deal with lists. But I don't have a strong opinion on the matter. I don't have an strong opinions on naming. Except that I'm bad at it. The limitation should at least be documented. (I didn't check of it already is) >>> + 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. > > Good point. I wasn't sure what is a reasonable maximum argument size. execve(2) says MAX_ARG_STRLEN (32 pages) which seems excessive. I could hardcode 8192 and be done with it. What do you think? > >> >>> + 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(). > > But then, I'd have to free it. I wanted to avoid this. The CLI utilities of util-linux generally don't free memory if the tool will exit soon anyways. >> >>> + >>> + /* 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 '&': >> } > > Good point. I'll do that in v2. > >> >>> + >>> + 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. > > Ack. > >> >>> + } else { >>> + if (cpulist_parse(arg, &bits, sizeof(bits), 1) < 0) >>> + return -EINVAL; >>> + } >>> + >>> + switch (op) { >>> + case OP_AND: >>> + copy = *all_bits; >>> + CPU_AND(all_bits, ©, &bits); >>> + break; >>> + case OP_OR: >>> + copy = *all_bits; >>> + CPU_OR(all_bits, ©, &bits); >>> + break; >>> + case OP_XOR: >>> + copy = *all_bits; >>> + CPU_XOR(all_bits, ©, &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? > > It may have been at some point but it isn't anymore. I'll remove it. > >> >>> + >>> + 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(). > > Since I am printing into a stack buffer, is there a benefit? Wouldn't the same hold true for a non-stack buffer? Anyways, this shouldn't be important. > >>> + 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()? > > Ack. > >> >>> + 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. > > I wanted to make ASAN happy. Is this not necessary? As mentioned above, freeing memory at the end of the process lifetime isn't really useful. The kernel will need to clean it up anyways. >>> + >>> + print_bits(&bits, mode); >>> + >>> + return EXIT_SUCCESS; >>> +} > > Thanks for the review!
