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
>
