On Mon, 28 Oct 2024, Ilpo Järvinen wrote:
> On Mon, 28 Oct 2024, Keith Busch wrote:
>
> > On Mon, Oct 28, 2024 at 07:40:45PM +0200, Ilpo Järvinen wrote:
> > > @@ -1430,7 +1431,7 @@ static ssize_t reset_method_store(struct device *dev,
> > > const char *buf, size_t count)
> > > {
> > > struct pci_dev *pdev = to_pci_dev(dev);
> > > - char *options, *tmp_options, *name;
> > > + char *tmp_options, *name;
> > > int m, n;
> > > u8 reset_methods[PCI_NUM_RESET_METHODS] = { 0 };
> > >
> > > @@ -1445,7 +1446,7 @@ static ssize_t reset_method_store(struct device *dev,
> > > return count;
> > > }
> > >
> > > - options = kstrndup(buf, count, GFP_KERNEL);
> > > + char *options __free(kfree) = kstrndup(buf, count, GFP_KERNEL);
> >
> > We should avoid mixing declarations with code. Please declare it with
> > the cleanup attribute at the top like before, and just initialize it to
> > NULL.
>
> Hi,
>
> I don't exactly disagree with you myself and would prefer to keep
> declarations at top, but I think as done now is exactly what Bjorn wants
> for the specific case where __free() is used. This was discussed earlier
> on the list.
Hi again,
I went to archives and found out it had already made itself into
include/linux/cleanup.h which now says this:
"
* Now, when a function uses both __free() and guard(), or multiple
* instances of __free(), the LIFO order of variable definition order
* matters. GCC documentation says:
*
* "When multiple variables in the same scope have cleanup attributes,
* at exit from the scope their associated cleanup functions are run in
* reverse order of definition (last defined, first cleanup)."
*
* When the unwind order matters it requires that variables be defined
* mid-function scope rather than at the top of the file.
[...snip examples...]
* Given that the "__free(...) = NULL" pattern for variables defined at
* the top of the function poses this potential interdependency problem
* the recommendation is to always define and assign variables in one
* statement and not group variable definitions at the top of the
* function when __free() is used.
"
After reading the documentation for real now myself :-), I realized it's
not just about maintainer preferences but about order of releasing things,
so it's a BAD PATTERN to put those declarations into the usual place when
using __free().
For completeness, the discussion thread (there might have been another
thread earlier than these):
https://lore.kernel.org/linux-pci/171140738438.1574931.15717256954707430472.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/T/#u
> If I misunderstood the conclusion of the earlier cleanup related
> discussion, can you please correct me Bjorn?
>
>
--
i.
