Re: [PATCH v3 1/2] tools/counter: add a flexible watch events tool

[William Breathitt Gray <william.gray@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 4671 bytes --]

On Wed, Dec 06, 2023 at 05:47:25PM +0100, Fabrice Gasnier wrote:
> This adds a new counter tool to be able to test various watch events.
> A flexible watch array can be populated from command line, each field
> may be tuned with a dedicated command line sub-option in "--watch" string.
> Several watch events can be defined, each can have specific watch options,
> by using "--watch <watch 1 options> --watch <watch 2 options>".
> Watch options is a comma separated list.
> 
> It also comes with a simple default watch (to monitor overflow/underflow
> events), used when no watch parameters are provided. It's equivalent to:
> counter_watch_events -w comp_count,scope_count,evt_ovf_udf
> 
> The print_usage() routine proposes another example, from the command line,
> which generates a 2 elements watch array, to monitor:
> - overflow underflow events
> - capture events, on channel 3, that reads read captured data by
>   specifying the component id (capture3_component_id being 7 here).
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> ---
> Changes in v3:
> - Free the allocated memory, also close the char device
> - Split of another patch series[1].
> [1] https://lore.kernel.org/lkml/20230922143920.3144249-1-fabrice.gasnier@foss.st.com/

Hi Fabrice,

Thank you for splitting this from the other patches. I think you may
still be leaking memory in a few places below.

> +	if (nwatch) {
> +		watches = calloc(nwatch, sizeof(*watches));
> +		if (!watches) {
> +			perror("Error allocating watches");
> +			return 1;
> +		}
> +	} else {
> +		/* default to simple watch example */
> +		watches = simple_watch;
> +		nwatch = ARRAY_SIZE(simple_watch);
> +	}

If we go down the calloc() path, then we should free the memory
before any return.

> +				case WATCH_CHANNEL:
> +					if (!value) {
> +						fprintf(stderr, "Missing chan=<number>\n");
> +						return -EINVAL;

Such as here.

> +					}
> +					watches[i].channel = strtoul(value, NULL, 10);
> +					if (errno)
> +						return -errno;

Here.

> +					break;
> +				case WATCH_ID:
> +					if (!value) {
> +						fprintf(stderr, "Missing id=<number>\n");
> +						return -EINVAL;

Here.

> +					}
> +					watches[i].component.id = strtoul(value, NULL, 10);
> +					if (errno)
> +						return -errno;

Here.

> +					break;
> +				case WATCH_PARENT:
> +					if (!value) {
> +						fprintf(stderr, "Missing parent=<number>\n");
> +						return -EINVAL;

Here.

> +					}
> +					watches[i].component.parent = strtoul(value, NULL, 10);
> +					if (errno)
> +						return -errno;

Here.

> +					break;
> +				default:
> +					fprintf(stderr, "Unknown suboption '%s'\n", value);
> +					return -EINVAL;

Here.

> +	ret = asprintf(&device_name, "/dev/counter%d", dev_num);
> +	if (ret < 0)
> +		return -ENOMEM;

Here.

> +	fd = open(device_name, O_RDWR);
> +	if (fd == -1) {
> +		perror("Unable to open counter device");
> +		return 1;

Here.

> +	}
> +
> +	for (i = 0; i < nwatch; i++) {
> +		ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + i);
> +		if (ret == -1) {
> +			fprintf(stderr, "Error adding watches[%d]: %s\n", i,
> +				strerror(errno));
> +			return 1;

Here.

> +		}
> +	}
> +
> +	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> +	if (ret == -1) {
> +		perror("Error enabling events");
> +		return 1;

Here.

> +	}
> +
> +	for (i = 0; loop <= 0 || i < loop; i++) {
> +		ret = read(fd, &event_data, sizeof(event_data));
> +		if (ret == -1) {
> +			perror("Failed to read event data");
> +			return 1;

Here.

> +		}
> +
> +		if (ret != sizeof(event_data)) {
> +			fprintf(stderr, "Failed to read event data\n");
> +			return -EIO;

And here.

> +	if (watches != simple_watch)
> +		free(watches);
> +	close(fd);
> +
> +	return 0;

We finally free watches here, close the file descriptor, and return. So
instead of returning an error code directly when you encounter a
problem, I would do an unwinding goto section like the following
instead.

First, the open() call occurs after the calloc(), so move the close()
call above the watches check so that we unwind in a first-in-last-out
order. Next, add a label to mark the file descriptor close section, and
another label to mark the watches free section. Then, rather than
returning 0 directly, return a retval that we can set. That way, when
you need to return on an error, set retval to the error code and goto
the file descriptor close label if we're past the open() call, or the
watches free label if we're just past the calloc() call.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]