On 11/15/24 02:55, Jan Beulich wrote:
> On 14.11.2024 19:50, Stewart Hildebrand wrote:
>> On 11/14/24 05:34, Jan Beulich wrote:
>>> On 12.11.2024 21:53, Stewart Hildebrand wrote:
>>>> + pdev->pf_pdev = pf_pdev;
>>>> + list_for_each_entry(vf_pdev, &pf_pdev->vf_list, vf_list)
>>>> + {
>>>> + if ( vf_pdev == pdev )
>>>> + {
>>>> + already_added = true;
>>>> + break;
>>>> + }
>>>> + }
>>>> + if ( !already_added )
>>>> + list_add(&pdev->vf_list, &pf_pdev->vf_list);
>>>> + }
>>>> }
>>>
>>> Personally, as I have a dislike for excess variables, I'd have gotten away
>>> without "already_added". Instead of setting it to true, vf_pdev could be
>>> set to NULL. Others may, however, consider this "obfuscation" or alike.
>>
>> This relies on vf_pdev being set to non-NULL when the list is empty and
>> after the last iteration if the list doesn't contain the element. I had
>> to look up the definitions of list_for_each_entry, INIT_LIST_HEAD, and
>> list_{add,del,entry} to verify that vf_pdev would be non-NULL in those
>> cases.
>>
>> Perhaps a better approach would be to introduce list_add_unique() in
>> Xen's list library? Then we can also get rid of the vf_pdev variable.
>>
>> static inline bool list_contains(struct list_head *entry,
>> struct list_head *head)
>> {
>> struct list_head *ptr;
>>
>> list_for_each(ptr, head)
>> {
>> if ( ptr == entry )
>> return true;
>> }
>>
>> return false;
>> }
>>
>> static inline void list_add_unique(struct list_head *new,
>> struct list_head *head)
>> {
>> if ( !list_contains(new, head) )
>> list_add(new, head);
>> }
>
> I'm uncertain of this kind of an addition. For long lists one would need to
> be careful with whether to actually use list_contains(). It being a simple
> library function would make this easy to overlook.
It occurs to me I could simply check if pdev->pf_pdev has been initialized:
if ( !pdev->pf_pdev )
list_add(&pdev->vf_list, &pf_pdev->vf_list);
pdev->pf_pdev = pf_pdev;
