On 27 Jan 2026, at 21:31, Ilya Maximets wrote:
> On 1/22/26 3:40 PM, Eelco Chaudron wrote:
>> When dumping a collection of offload providers, take a
>> reference to prevent the collection from being destroyed.
>>
>> Fixes: 7eb9ffac52e4 ("dpif-offload-provider: Add dpif-offload-provider
>> implementation.")
>> Signed-off-by: Eelco Chaudron <[email protected]>
>> ---
>> lib/dpif-offload.c | 103 +++++++++++++++++++++++++--------------------
>> lib/dpif-offload.h | 3 +-
>> 2 files changed, 60 insertions(+), 46 deletions(-)
>
> Thanks, Eelco! This looks good to me in general. A couple comments below.
[...]
>> if (dump->state) {
>> - struct dpif_offload *offload_entry;
>> - bool valid_member = false;
>> -
>> - /* In theory, list entries should not be removed. However, in case
>> - * someone calls this during destruction and the node has
>> disappeared,
>> - * we will return EIDRM (Identifier removed). */
>> - LIST_FOR_EACH (offload_entry, dpif_list_node, &collection->list) {
>> - if (offload_entry == dump->state) {
>> - valid_member = true;
>> - break;
>> - }
>> - }
>> -
>> - if (valid_member) {
>> - offload_entry = dump->state;
>> + struct dpif_offload *offload_entry =dump->state;
>
> nit: Missing space after the '='.
Will fix
[...]
>> diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h
>> index 65bba2f0ed..c4e6284a26 100644
>> --- a/lib/dpif-offload.h
>> +++ b/lib/dpif-offload.h
>> @@ -20,12 +20,13 @@
>> #include "dpif.h"
>>
>> /* Forward declarations of private structures. */
>> +struct dpif_offload_provider_collection;
>> struct dpif_offload_class;
>> struct dpif_offload;
>>
>> /* Structure used by the dpif_offload_dump_* functions. */
>> struct dpif_offload_dump {
>> - const struct dpif *dpif;
>> + const struct dpif_offload_provider_collection *collection;
>> int error;
>> void *state;
> Shouldn't collection be part of the state? Feels like it should be,
> as this is a "public" structure that shouldn't expose internal types,
> hence the reason the 'state' is a void *.
>
> We could define a new struct dpif_offload_dump_state inside the .c
> file with two fields - 'collection' and 'entry'. And do a dump->state
> cast first thing in the functions and then use it afterwards. May also
> be possible to use the state->entry directly in the list iterators as
> it will have a proper type.
>
> WDYT?
I’ll rework it to use a void *state, so the whole structure can go away. Same
as the dpif port dump APIs.
Sent the v2 once I’m done.
//Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
