Re: [PATCH 2/2] staging: gpib: Agilent usb code cleanup

[Dave Penkler]

On Mon, Jan 20, 2025 at 10:15:55AM +0300, Dan Carpenter wrote:
> This patch does too many things...  It should be split up.  People
> complain about this requirement a lot, but eventually it will become
> instinctive.  I use `git citool` so I can highlight and click to add
> lines to a commit.  In this code there were some dev_info() changes
> mixed into the unwind code in ->attach() that were hard to separate out
> into their own commit but it wasn't too complicated.
> 
> On Sat, Jan 18, 2025 at 03:50:46PM +0100, Dave Penkler wrote:
> > Remove useless #ifdef RESET_USB_CONFIG code.
> > 
> 
> patch 1.
> 
> > Change kalloc / memset to kzalloc
> > 
> 
> patch 2.
> 
> > The attach function was not freeing the private data on error
> > returns. Separate the releasing of urbs and private data and
> > add a common error exit for attach failure.
> > 
> > Set the board private data pointer to NULL after freeing
> > the private data.
> 
> By setting the private data, this patch actually does fix the
> double free that I mentioned earlier.  It changes the ->detach into
> a no-op if ->attach fails.  Needs a Fixes tag.  ;)
> 
> But I still hope my blog will convince you that the error handling can be
> re-written in a better way.  It shouldn't matter if ->private_data is
> NULL or non-NULL because the caller should only have to handle success
> or failure.  The caller shouldn't have to handle a dozen different
> failure modes:
> 
> 1) Failure but the ->private_data is NULL
> 2) Failure but the foo->frob pointer is an error pointer
> 3) Failure but the foo->frob pointer needs to be freed.
> 4) Failure but the foo->frob pointer contains other pointers which
>    need to be freed.
> 5) ...
> 
> It should just be
> 
> 1) Success: Everything is allocated
> 2) Failure: Everything is cleaned up and any accesses are probably a
>    use after free.
> 
> > 
> > Reduce console spam by emitting only one attach message.
> > 
> > Change last pr_err in attach to dev_err
> > 
> 
> These last two can probably be combined into one patch?
> 
> > @@ -1388,11 +1367,19 @@ static int agilent_82357a_attach(gpib_board_t *board, const gpib_board_config_t
> >  	retval = agilent_82357a_init(board);
> >  
> >  	if (retval < 0)	{
> > -		mutex_unlock(&agilent_82357a_hotplug_lock);
> > -		return retval;
> > +		agilent_82357a_cleanup_urbs(a_priv);
> > +		agilent_82357a_release_urbs(a_priv);
> > +		goto attach_fail;
> >  	}
> 
> In my blog talk about how every allocation function should have a
> matching free() function.  These two functions match
> agilent_82357a_setup_urbs() so we should have a single function to
> release the urbs.
Hi,
I fully agree with you and this is the direction we are pursuing in
the gpib driver code base. We have very long way to go still and
I apologize for not splitting up the changes into multiple patches.
Thanks for the pointer to git citool.
-dave


> 
> regards,
> dan carpenter
>