Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 11 Dec 2023 15:57:28 +0000
From:   William Breathitt Gray <william.gray@linaro.org>
To:     Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Cc:     linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/2] tools/counter: add a flexible watch events tool
Message-ID: <ZXcxaFRuGJg6kYuT@ubuntu-server-vm-macos>
References: <20231206164726.418990-1-fabrice.gasnier@foss.st.com>
 <20231206164726.418990-2-fabrice.gasnier@foss.st.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="DgK1nOAauTq8w6Yp"
Content-Disposition: inline
In-Reply-To: <20231206164726.418990-2-fabrice.gasnier@foss.st.com>
Precedence: bulk


--DgK1nOAauTq8w6Yp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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
>=20
> 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).
>=20
> 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 =3D calloc(nwatch, sizeof(*watches));
> +		if (!watches) {
> +			perror("Error allocating watches");
> +			return 1;
> +		}
> +	} else {
> +		/* default to simple watch example */
> +		watches =3D simple_watch;
> +		nwatch =3D 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=3D<number>\n");
> +						return -EINVAL;

Such as here.

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

Here.

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

Here.

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

Here.

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

Here.

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

Here.

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

Here.

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

Here.

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

Here.

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

Here.

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

Here.

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

Here.

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

And here.

> +	if (watches !=3D 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

--DgK1nOAauTq8w6Yp
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQSNN83d4NIlKPjon7a1SFbKvhIjKwUCZXcxaAAKCRC1SFbKvhIj
KyH7AQDS6pGsvZkqLTWO56L/iANt86BNInqaN0ixwVhpsDcpBgD/dVb/3cacHHOH
q1S50LhsUQ4jogqKhU20axDclAOqvQQ=
=3Fgn
-----END PGP SIGNATURE-----

--DgK1nOAauTq8w6Yp--