[lustre-devel] [PATCH 05/21] lustre: use list_first_entry() in lustre subdirectory.

[NeilBrown]

On Fri, Feb 08 2019, Andreas Dilger wrote:

> On Feb 6, 2019, at 17:03, NeilBrown <neilb at suse.com> wrote:
>> 
>> Convert
>>  list_entry(foo->next .....)
>> to
>>  list_first_entry(foo, ....)
>> 
>> in 'lustre'
>> 
>> In several cases the call is combined with
>> a list_empty() test and list_first_entry_or_null() is used
>> 
>> Signed-off-by: NeilBrown <neilb at suse.com>
>
> One question below:
>
>> @@ -912,9 +913,9 @@ static int ll_agl_thread(void *arg)
>> 
>> 	spin_lock(&plli->lli_agl_lock);
>> 	sai->sai_agl_valid = 0;
>> -	while (!list_empty(&sai->sai_agls)) {
>> -		clli = list_entry(sai->sai_agls.next,
>> -				  struct ll_inode_info, lli_agl_list);
>> +	while ((clli = list_first_entry_or_null(&sai->sai_agls,
>> +						struct ll_inode_info,
>> +						lli_agl_list)) != NULL) {
>
> Lustre coding style used to require explicit comparisons against NULL or 0
> to avoid subtle bugs when developers treat pointers like booleans, so I'm
> not against this, but the kernel style is to not do this, like:
>
> 	while ((clli = list_first_entry_or_null(&sai->sai_agls,
> 						struct ll_inode_info,
> 						lli_agl_list))) {
>
> Will there be grief when this is pushed upstream?  In that case, it might
> be better to just use the implicit "!= NULL" and avoid the complaints.
>

I don't think there will be grief.  I personally prefer the explicit
test, and I think I recall Linus once saying he did too.

If you have
  while (p = func()) {

The compile will warn that maybe you meant '=='.
So you need

  while ((p = func())) {

and the extra parentheses look odd.  Make it

   while ((p = func()) != NULL) {

clears it all up.

The pattern
  while.* = .* != NULL

occurs 589 times in the kernel at present including fs/* mm/* net/*
so we certainly would be alone in using it.
  

> Either way, the technical aspects of the patch are OK, so:
>
> Reviewed-by: Andreas Dilger <adilger at whamcloud.com>

Thanks,
NeilBrown

>
>
> Cheers, Andreas
> ---
> Andreas Dilger
> Principal Lustre Architect
> Whamcloud
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20190211/6a5e0587/attachment.sig>