On 12/22/2012 01:47 PM, Alexander Nezhinsky wrote:
>> +static inline int list_is_last(const struct list_head *list,
>> + const struct list_head *head)
>> +{
>> + return list->next == head;
>> +}
>
> Can't we just use list_add() instead of list_add_tail() in spc_pr_register():
> list_add_tail(®->registration_siblings,
> &cmd->dev->registration_list);
> then we don't need to introduce list_is_last() and manage it with
> list_first_entry() ?
I believe I implemented this incorrectly. What I really wanted to know
was "is this the only entry in the last", not "is this the last entry
in the list of possibly many entries".
I will rework the patch. Right now, my patch looks to see if the registration
that is about to be deleted is the last one. But I believe it would be
better to delete the registration, then check to see if the list is
empty. Then I can use the existing list_empty() function.
I don't believe using list_add() vs list_add_tail() is relevant given
these facts.
>
>
>> +static void __unregister_and_clean(struct scsi_cmd *cmd,
>> + struct registration *reg)
>> +{
> . . .
>> + if (!is_pr_type_all_registrants(holder->pr_type) ||
>> + list_is_last(®->registration_siblings,
>> + ®->registration_siblings)) {
>
> Isn't it supposed to be:
>> + list_is_last(®->registration_siblings,
>> + &cmd->dev->registration_list)) {
> ?
Yes, I believe you are right, but that code will change with an
updated patch.
>
> Another thing, i'd suggest making a separate patch for
> is_pr_type_all_registrants()
> because it is a refactoring-like stuff and quite separate from other additions
> and fixes.
>
> Alexander
I will remove the is_pr_type_all_registrants() function, obviating
the need for a second patch. I thought it made the code more readable,
but I can understand your dislike for "refactoring-like stuff".
I will submit an updated patch soon.
--
Lee Duncan
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
