Hi Matt, Thanks for fixing this one. Yes this seems like a sound plan!
Best regards, Ole > On 8 Apr 2026, at 21:24, Matthew Smith via lists.fd.io > <[email protected]> wrote: > > > Hi, > > A patch went in a while back that broke my client code which sends the > gre_tunnel_dump API and processes the replies. Here's the patch - > https://github.com/FDio/vpp/commit/74cf96576768f6e9200848c613428990049b3563. > > This was discovered a couple of months ago when we updated our build from > stable/2506 to stable/2510. At the time, we just applied a patch locally to > make things work because we did not have time to properly fix it. I'm trying > to update again from stable/2510 to stable/2602 so I thought I should > probably fix it upstream for real this time. > > Here are some details on the issues: > > The patch adds a global 'option status = "in_progress";'. I have seen that > option on individual API messages but I have no idea whether this has any > meaning when declared globally. Is this a valid usage or is it just being > silently ignored by vppapigen? > > It also adds this service definition: > > service { > rpc gre_tunnel_dump returns gre_tunnel_dump_reply events gre_tunnel_details; > rpc gre_tunnel_dump_v2 returns gre_tunnel_dump_v2_reply events > gre_tunnel_details_v2; > }; > > I don't think "events" is correct there. I think if the API handlers for > gre_tunnel_dump and gre_tunnel_dump_v2 used a cursor and the macros to send > details messages in batches, it might be appropriate to use "stream" there > instead of event. As it is, those API handlers don't do that cursor magic, > they just iterate all GRE tunnels and send details messages without sending > the reply message (i.e. typical dump/details behavior), so really this > service definition and the dump_reply/dump_reply_v2 message types might be > entirely useless. > > I don't think vppapigen does the right thing with the names > gre_tunnel_dump_v2 and gre_tunnel_details_v2. The script expects > _dump/_details at the end of a dump/details API name. The new APIs should be > called gre_tunnel_v2_dump/gre_tunnel_v2_details. Random non sequitur - there > is one other dump/details message that likely has this problem - > policer_dump_v2. > > So... I'm trying to figure out how to make things work correctly again. > Ideally we could revert the patch and tell the author "try again". But it was > merged > 6 months ago and it adds new functionality (support for GRE key), > which I assume someone somewhere might care about and would not like to > disappear. So just reverting it is probably not the nice thing to do at this > point. So the second best option here IMO would be: > Remove the global option status in_progress. Even if it's a valid > configuration, I don't think the GRE APIs, which were formerly considered > production, should be considered "in progress". > Put gre_tunnel_dump back the way it was before (remove it from that service > rpc definition, remove or deprecate the unused gre_tunnel_dump_reply) > Either rename the new APIs or add new ones with the correct names and mark > the bad ones deprecated so they can be removed later. > Does this seem like a sound plan? Is any of this going to trip up the crc > checker script? Is there some reason why the stuff I wrote above is wrong and > this patch is good and I should leave it alone? > > Thanks, > -Matt > > > >
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#26937): https://lists.fd.io/g/vpp-dev/message/26937 Mute This Topic: https://lists.fd.io/mt/118730935/21656 Group Owner: [email protected] Unsubscribe: https://lists.fd.io/g/vpp-dev/leave/14379924/21656/631435203/xyzzy [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
