linux-kernel - Re: [PATCH 08/25] staging: lustre: libcfs: add cpu distance handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180416144548.xjjcaxivkbvynsux@mwanda>
Date:   Mon, 16 Apr 2018 17:45:48 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     James Simmons <jsimmons@...radead.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org,
        Andreas Dilger <andreas.dilger@...el.com>,
        Oleg Drokin <oleg.drokin@...el.com>,
        NeilBrown <neilb@...e.com>,
        Amir Shehata <amir.shehata@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lustre Development List <lustre-devel@...ts.lustre.org>
Subject: Re: [PATCH 08/25] staging: lustre: libcfs: add cpu distance handling

On Mon, Apr 16, 2018 at 12:09:50AM -0400, James Simmons wrote:
> +int cfs_cpt_distance_print(struct cfs_cpt_table *cptab, char *buf, int len)
> +{
> +	char *tmp = buf;
> +	int rc = -EFBIG;
> +	int i;
> +	int j;
> +
> +	for (i = 0; i < cptab->ctb_nparts; i++) {
> +		if (len <= 0)
> +			goto err;
> +
> +		rc = snprintf(tmp, len, "%d\t:", i);
> +		len -= rc;
> +
> +		if (len <= 0)
> +			goto err;


The "goto err" here is sort of an example of a "do-nothing" goto.  There
are no resources to free or anything like that.

I don't like do-nothing gotos because "return -EFBIG;" is self
documenting code and "goto err;" is totally ambiguous what it does.  The
second problem is that do-nothing error labels easily turn into
do-everything one err style error paths which I loath.  And the third
problem is that they introduce "forgot to set the error code" bugs.

It looks like we forgot to set the error code here although it's also
possible that was intended.  This code is ambiguous.

> +
> +		tmp += rc;
> +		for (j = 0; j < cptab->ctb_nparts; j++) {
> +			rc = snprintf(tmp, len, " %d:%d",
> +				      j, cptab->ctb_parts[i].cpt_distance[j]);
> +			len -= rc;
> +			if (len <= 0)
> +				goto err;
> +			tmp += rc;
> +		}
> +
> +		*tmp = '\n';
> +		tmp++;
> +		len--;
> +	}
> +	rc = 0;
> +err:
> +	if (rc < 0)
> +		return rc;
> +
> +	return tmp - buf;
> +}

regards,
dan carpenter