> On 13. Apr 2022, at 19:05, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> On Fri, 1 Apr 2022 at 00:11, Jakob Koschel <jakobkoschel@xxxxxxxxx> wrote:
>>
>> In preparation to limiting the scope of a list iterator to the list
>> traversal loop, use a dedicated pointer to iterate through the list [1].
>>
>> In the current state the list_for_each_entry() is guaranteed to
>> hit a break or goto in order to work properly. If the list iterator
>> executes completely or the list is empty the iterator variable contains
>> a type confused bogus value infered from the head of the list.
>>
>> With this patch the variable used past the list iterator is only set
>> if the list exists early and is NULL otherwise. It should therefore
>> be safe to just set *prev = NULL (as it was before).
>>
>
> This generic boilerplate is fine to include, but it would help if you
> could point out why repainting the current logic with your new brush
> is appropriate here.
This makes sense, I can see that the commit message should be improved here.
>
> In this particular case, I wonder whether updating *prev makes sense
> to begin with if we are returning an error, and if we fix that, the
> issue disappears as well.
Actually I'm rethinking this now. The only use of 'prev' that I can see is
in efi_pstore_erase_name(). It only uses it if found != 0
which would mean err != 0 in __efivar_entry_iter().
This would allow massively simplifying the entire function.
The valid case is updating *prev when there is an "error" as far as I can tell.
I've sketched up a rewritten function that should hopefully be more clear and
archive the same goal, I'm curious what you think:
int __efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
struct list_head *head, void *data,
struct efivar_entry **prev)
{
struct efivar_entry *entry, *n;
int err = 0;
/* If prev is set and *prev != NULL start iterating from there */
if (prev)
entry = list_prepare_entry(*prev, head, list);
/* Otherwise start at the beginning */
else
entry = list_entry(head, typeof(*entry), list);
list_for_each_entry_safe_continue(entry, n, head, list) {
err = func(entry, data);
if (err && prev)
*prev = entry;
if (err)
return err;
}
return 0;
}
