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
