linux-kernel - Re: [PATCH linux-next] drm/gem: Remove the unneeded result variable

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] [day] [month] [year] [list]

Date:   Wed, 31 Aug 2022 14:02:54 +0200
From:   Thierry Reding <treding@...dia.com>
To:     Jani Nikula <jani.nikula@...ux.intel.com>
Cc:     cgel.zte@...il.com, airlied@...ux.ie, tzimmermann@...e.de,
        ye xingchen <ye.xingchen@....com.cn>,
        Zeal Robot <zealci@....com.cn>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, Maxime Ripard <maxime@...no.tech>,
        Julia Lawall <julia.lawall@...ia.fr>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH linux-next] drm/gem: Remove the unneeded result variable

On Wed, Aug 31, 2022 at 12:25:52PM +0300, Jani Nikula wrote:
> On Wed, 31 Aug 2022, cgel.zte@...il.com wrote:
> > From: ye xingchen <ye.xingchen@....com.cn>
> >
> > Return the value drm_gem_handle_delete() directly instead of storing it
> > in another redundant variable.
> 
> Personally, I generally prefer keeping these types of variables, because
> they make future changes on the code easier. For example, adding a new
> call where you need to check the return value, or having to deal with
> the error value right there. Or just adding ad hoc debug logging of the
> return value.
> 
> Related, but not exactly the same, see commit b784c7707502
> ("coccinnelle: Remove ptr_ret script"). Adding Cc's from that commit for
> input.
> 
> BR,
> Jani.
> 
> > Reported-by: Zeal Robot <zealci@....com.cn>
> > Signed-off-by: ye xingchen <ye.xingchen@....com.cn>
> > ---
> >  drivers/gpu/drm/drm_gem.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index ad068865ba20..3fa0deff3014 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -782,14 +782,11 @@ drm_gem_close_ioctl(struct drm_device *dev, void *data,
> >  		    struct drm_file *file_priv)
> >  {
> >  	struct drm_gem_close *args = data;
> > -	int ret;
> >  
> >  	if (!drm_core_check_feature(dev, DRIVER_GEM))
> >  		return -EOPNOTSUPP;
> >  
> > -	ret = drm_gem_handle_delete(file_priv, args->handle);
> > -
> > -	return ret;
> > +	return drm_gem_handle_delete(file_priv, args->handle);
> >  }
> >  
> >  /**

I agree with Jani here. If I recall correctly, the pattern that the
coccinelle script was dealing with was slightly different and in this
example would've looked like this.

	ret = drm_gem_handle_delete(file_priv, args->handle);
	if (ret)
		return ret;

	return 0;

You can collapse that in the same way, but I think it makes it even
clearer than the original why it makes sense to keep this construct.

If you even need to add any other cleanup code between the "return ret;"
and "return 0;" lines, you can do so without rearranging the rest of the
code.

It's obviously highly subjective which one is preferable, which is why
the recommendation at the time was not to provide a script that would
make any suggestions as to the "right" style.

In my opinion, if you write the code in the condensed form, that's fine,
but if you prefer the more verbose variant, that's equally fine. Trying
to "fix" this up after the fact for no other reason than to save a few
lines and/or a variable, however, is a complete waste of time.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)