On 10/17/12 22:47, Vivek Goyal wrote:
> On Wed, Oct 17, 2012 at 09:02:22AM +0900, Jun'ichi Nomura wrote:
>> On 10/17/12 08:20, Tejun Heo wrote:
>>>>>> - if (ent == &q->root_blkg->q_node)
>>>>>> + if (q->root_blkg && ent == &q->root_blkg->q_node)
>>>>>
>>>>> Can we fix it little differently. Little earlier in the code, we check for
>>>>> if q->blkg_list is empty, then all the groups are gone, and there are
>>>>> no more request lists hence and return NULL.
>>>>>
>>>>> Current code:
>>>>> if (rl == &q->root_rl) {
>>>>> ent = &q->blkg_list;
>>>>>
>>>>> Modified code:
>>>>> if (rl == &q->root_rl) {
>>>>> ent = &q->blkg_list;
>>>>> /* There are no more block groups, hence no request lists */
>>>>> if (list_empty(ent))
>>>>> return NULL;
>>>>> }
>>>
>>> Do we need this at all? q->root_blkg being NULL is completely fine
>>> there and the comparison would work as expected, no?
>>
>> Hmm?
>>
>> If list_empty(ent) and q->root_blkg == NULL,
>>
>>> /* walk to the next list_head, skip root blkcg */
>>> ent = ent->next;
>>
>> ent is &q->blkg_list again.
>>
>>> if (ent == &q->root_blkg->q_node)
>>
>> So ent is not &q->root_blkg->q_node.
>
> If q->root_blkg is NULL, will it not lead to NULL pointer dereference.
> (q->root_blkg->q_node).
It's not dereferenced.
>>> ent = ent->next;
>>> if (ent == &q->blkg_list)
>>> return NULL;
>>
>> And we return NULL here.
>>
>> Ah, yes. You are correct.
>> We can do without the above hunk.
>
> I would rather prefer to check for this boundary condition early and
> return instead of letting it fall through all these conditions and
> then figure out yes we have no next rl. IMO, code becomes easier to
> understand if nothing else. Otherwise one needs a step by step
> explanation as above to show that case of q->root_blkg is covered.
I have same opinion as yours that it's good for readability.
--
Jun'ichi Nomura, NEC Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
