Re: git: d836c48e7110 - main - cam_periph: wired is really a bool, update it to a bool.

On 11/5/21, Warner Losh <imp@freebsd.org> wrote:
> The branch main has been updated by imp:
>
> URL:
> https://cgit.FreeBSD.org/src/commit/?id=d836c48e7110f2894885cf84ce8990f7916663cc
>
> commit d836c48e7110f2894885cf84ce8990f7916663cc
> Author:     Warner Losh <imp@FreeBSD.org>
> AuthorDate: 2021-11-05 14:56:48 +0000
> Commit:     Warner Losh <imp@FreeBSD.org>
> CommitDate: 2021-11-05 14:56:48 +0000
>
>     cam_periph: wired is really a bool, update it to a bool.
>
>     Sponsored by:           Netflix
>     Reviewed by:            scottl
>     Differential Revision:  https://reviews.freebsd.org/D32823
> ---
>  sys/cam/cam_periph.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/sys/cam/cam_periph.c b/sys/cam/cam_periph.c
> index bb4baaf0888f..54fe9a0ef40c 100644
> --- a/sys/cam/cam_periph.c
> +++ b/sys/cam/cam_periph.c

> @@ -600,8 +600,9 @@ static u_int
>  camperiphunit(struct periph_driver *p_drv, path_id_t pathid,
>      target_id_t target, lun_id_t lun, const char *sn)
>  {
> +	bool	wired;
>  	u_int	unit;
> -	int	wired, i, val, dunit;
> +	int	i, val, dunit;
>  	const char *dname, *strval;
>  	char	pathbuf[32], *periph_name;
>
> @@ -610,29 +611,29 @@ camperiphunit(struct periph_driver *p_drv, path_id_t
> pathid,
>  	unit = 0;
>  	i = 0;
>  	dname = periph_name;
> -	for (wired = 0; resource_find_dev(&i, dname, &dunit, NULL, NULL) == 0;
> -	     wired = 0) {
> +	while (resource_find_dev(&i, dname, &dunit, NULL, NULL) == 0) {
> +		wired = false;

This has a side effect of no longer initializing wired if the first
resource_find_dev call returns != 0, which in turn causes KMSAN to
panic due to:

        unit = camperiphnextunit(p_drv, unit, wired, pathid, target, lun);

below.

It is unclear to me what this code should do. Plopping 'wired =
false;' upfront at least restores the previous state and prevents the
panic.

>  		if (resource_string_value(dname, dunit, "at", &strval) == 0) {
>  			if (strcmp(strval, pathbuf) != 0)
>  				continue;
> -			wired++;
> +			wired = true;
>  		}
>  		if (resource_int_value(dname, dunit, "target", &val) == 0) {
>  			if (val != target)
>  				continue;
> -			wired++;
> +			wired = true;
>  		}
>  		if (resource_int_value(dname, dunit, "lun", &val) == 0) {
>  			if (val != lun)
>  				continue;
> -			wired++;
> +			wired = true;
>  		}
>  		if (resource_string_value(dname, dunit, "sn", &strval) == 0) {
>  			if (sn == NULL || strcmp(strval, sn) != 0)
>  				continue;
> -			wired++;
> +			wired = true;
>  		}
> -		if (wired != 0) {
> +		if (wired) {
>  			unit = dunit;
>  			break;
>  		}
>


-- 
Mateusz Guzik <mjguzik gmail.com>