Re: [PATCH 3/3] commands: watch: add new command

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Jul 01, 2024 at 09:54:34AM +0200, Ahmad Fatoum wrote:
> For testing proper operation of IIO devices, it can be useful to monitor
> changes in the reading reported by the hwmon command. This is now
> possible by using `watch -n 0.5 -t hwmon`.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  commands/Kconfig  |  6 +++
>  commands/Makefile |  1 +
>  commands/watch.c  | 97 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+)
>  create mode 100644 commands/watch.c
> 
> diff --git a/commands/Kconfig b/commands/Kconfig
> index c9c4be67e098..5b512f1bbac7 100644
> --- a/commands/Kconfig
> +++ b/commands/Kconfig
> @@ -2417,6 +2417,12 @@ config CMD_TIME
>  	  Note: This command depends on COMMAND being interruptible,
>  	  otherwise the timer may overrun resulting in incorrect results
>  
> +config CMD_WATCH
> +	bool "watch"
> +	help
> +	  watch is used to execute a command periodically, showing
> +	  output to the screen.
> +
>  config CMD_UPTIME
>  	bool "uptime"
>  	help
> diff --git a/commands/Makefile b/commands/Makefile
> index f3e8e944a931..4ca7ba7eb609 100644
> --- a/commands/Makefile
> +++ b/commands/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_CMD_WD)		+= wd.o
>  obj-$(CONFIG_CMD_LED_TRIGGER)	+= trigger.o
>  obj-$(CONFIG_CMD_USB)		+= usb.o
>  obj-$(CONFIG_CMD_TIME)		+= time.o
> +obj-$(CONFIG_CMD_WATCH)		+= watch.o
>  obj-$(CONFIG_CMD_UPTIME)	+= uptime.o
>  obj-$(CONFIG_CMD_OFTREE)	+= oftree.o
>  obj-$(CONFIG_CMD_OF_COMPATIBLE)	+= of_compatible.o
> diff --git a/commands/watch.c b/commands/watch.c
> new file mode 100644
> index 000000000000..84731eb8505a
> --- /dev/null
> +++ b/commands/watch.c
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Port of Mini watch implementation from busybox
> + *
> + * Copyright (C) 2001 by Michael Habermann <mhabermann@gmx.de>
> + * Copyrigjt (C) Mar 16, 2003 Manuel Novoa III   (mjn3@codepoet.org)
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <clock.h>
> +#include <linux/math64.h>
> +#include <malloc.h>
> +#include <getopt.h>
> +#include <term.h>
> +#include <rtc.h>
> +
> +static int do_watch(int argc , char *argv[])
> +{
> +	const char *period_str = "2.0";
> +	u64 period_ns, start;
> +	bool print_header = false;
> +	int opt;
> +	unsigned width, new_width;
> +	char *end, *header, *cmd;
> +
> +	while ((opt = getopt(argc, argv, "+n:t")) > 0) {
> +		switch (opt) {
> +		case 'n':
> +			period_str = optarg;
> +			break;
> +		case 't':
> +			print_header = true;
> +			break;
> +		default:
> +			return COMMAND_ERROR_USAGE;
> +		}
> +	}
> +
> +	argc -= optind;
> +	argv += optind;
> +
> +	if (argc < 1)
> +		return COMMAND_ERROR_USAGE;
> +
> +	cmd = strjoin(" ", argv, argc);
> +
> +	period_ns = simple_strtofract(period_str, &end, NSEC_PER_SEC);
> +	if (*end)
> +		return -EINVAL;
> +
> +	width = (unsigned)-1; // make sure first time new_width != width
> +	header = NULL;
> +
> +	while (!ctrlc()) {
> +		/* home; clear to the end of screen */
> +		printf("\e[H\e[J");
> +
> +		if (print_header) {
> +			term_getsize(&new_width, NULL);
> +			if (new_width != width) {
> +				width = new_width;
> +				free(header);
> +				header = xasprintf("Every %ss: %-*s",
> +						   period_str, (int)width, cmd);

header should be freed in the exit path.

> +			}
> +
> +			printf("%s\n\n", header);
> +		}
> +
> +		run_command(cmd);
> +
> +		start = get_time_ns();
> +		while (!is_timeout(start, period_ns)) {
> +			if (ctrlc())
> +				return 1;

cmd should be freed here.

I think the return value should be 0 here like in the other exit path.

> +		}
> +	}
> +
> +	free(cmd);
> +
> +	return 0;
> +}
> +
> +BAREBOX_CMD_HELP_START(watch)
> +BAREBOX_CMD_HELP_TEXT("Options:")
> +BAREBOX_CMD_HELP_OPT ("-n SEC", "Period (default 2)")
> +BAREBOX_CMD_HELP_OPT ("-t",	"Don't print header")

The implementation does the opposite of what the description says.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |