The GVariants in the hash table returned by nm_utils_parse_variant_attributes() are floating. This is unexpected in my opinion. We should at least document the current behavior, and also introduce a new API with the correct one (GVariant have one reference and are not floating).
See also https://bugzilla.redhat.com/show_bug.cgi?id=1555013#c9
Pushed branch bg/parse-var-attr-ref-rh1594887 for review.
> clients: fix memory leak when parsing routes
why was this not caught be unit-tests and valgind? Could you extend the unit-tests, so that the leak shows up?
I know, we discussed this already, and there was a consensus. Fair enough, if everybody else agrees with this approach. For the record, I wouldn't do this, because:
Too bad we missed 1.12.0 with these fixes. So, by ~fixing~ API now, we not only break behaviour for 1.10.x, but also 1.12.0. I don't like that. I a library introduced a bug, it's first and foremost the responsibility of the library (developers) to take the burden to improve the situation.
Especially as in this case. The previous behaviour -- while odd -- was indeed used correctly (by callers sinking the reference). If this would be a bug, which unavoidable lead to leak/memory-corruption, than of course the API can only be fixed. But the old API did not result in buggy behaviour, that's why I think it would be lazy of us, to just change behaviour and call it a fix. We should pride ourself of not changing behaviour unless unavoidable. That doesn't seem to be the case here.
See upstream commits:
The first, is cherry-picked (and modified) from bg/parse-var-attr-ref-rh1594887 branch.
This missed rhel-7.6.
It also missed rhel-8.0 (aside the bits from comment 4).
So the only reasonable approach would to add a new API with the new (better?) behavior, but it seems a waste of effort as the current API is fine and its behavior well documented. I'm going to close this.