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

[Fabrice Gasnier]

On 12/11/23 16:57, William Breathitt Gray wrote:
> 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@xxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxx/
> 
> 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.

Hi William,

Ah yes, I missed that. I'll fix it in later revision, Thanks!

Best Regards,
Fabrice

> 
>> +				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