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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <daecc634-4faf-4dcb-b03b-f57f24673a88@stanley.mountain>
Date: Thu, 29 Aug 2024 12:36:25 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Philipp Hortmann <philipp.g.hortmann@...il.com>
Cc: Manisha Singh <masingh.linux@...il.com>,
	florian.c.schilhabel@...glemail.com, gregkh@...uxfoundation.org,
	linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] staging: rtl8712: Fix style issues in rtl871x_io.c

On Wed, Aug 28, 2024 at 11:08:31PM +0200, Philipp Hortmann wrote:
> > diff --git a/drivers/staging/rtl8712/rtl871x_io.c b/drivers/staging/rtl8712/rtl871x_io.c
> > index 6789a4c98564..6311ac15c581 100644
> > --- a/drivers/staging/rtl8712/rtl871x_io.c
> > +++ b/drivers/staging/rtl8712/rtl871x_io.c
> > @@ -48,10 +48,10 @@ static uint _init_intf_hdl(struct _adapter *padapter,
> >   	set_intf_funs = &(r8712_usb_set_intf_funs);
> >   	set_intf_ops = &r8712_usb_set_intf_ops;
> >   	init_intf_priv = &r8712_usb_init_intf_priv;
> > -	pintf_priv = pintf_hdl->pintfpriv = kmalloc(sizeof(struct intf_priv),
> > -						    GFP_ATOMIC);
> > +	pintf_priv = kmalloc(sizeof(struct intf_priv), GFP_ATOMIC);
> >   	if (!pintf_priv)
> >   		goto _init_intf_hdl_fail;
> 
> By pushing the below statement after the "if (!pintf_priv)" you change the
> logic. Is this really wanted? Why do you think it is better? I would avoid
> this and it would be a separate patch anyhow.
> 
> > +	pintf_hdl->pintfpriv = pintf_priv;

I liked moving it.  And I feel like it should be done in this patch, not as a
separate patch.  But it should have some justification as you say.  The commit
message could say something like:

    Checkpatch complains that we should avoid multiple assignments on the same
    line for readability purposes.  Generally, we would allocate, check and then
    assign.  It doesn't matter what is assigned to "pintf_hdl->pintfpriv" on the
    error path.  For example, on subsequent error paths "pintf_hdl->pintfpriv"
    is a freed pointer.  So this code is okay as-is and it's also okay to move
    the pintf_hdl->pintfpriv = pintf_priv assignment after the NULL check.

(Notice how I sold this as one thing, moving the "pintf_hdl->pintfpriv"
assignment, not silencing checkpatch and then moving it.  Notice how I avoided
using the word "also".)

regards,
dan carpenter


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ