Bug 1594887

Summary: nm_utils_parse_variant_attributes() returns floating GVariants
Product: Red Hat Enterprise Linux 7 Reporter: Beniamino Galvani <bgalvani>
Component: NetworkManagerAssignee: Beniamino Galvani <bgalvani>
Status: CLOSED NOTABUG QA Contact: Desktop QE <desktop-qa-list>
Severity: medium Docs Contact:
Priority: medium    
Version: 7.5CC: atragler, bgalvani, fgiudici, lrintel, rkhan, sukulkar, thaller
Target Milestone: rc   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2018-11-22 17:34:55 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description Beniamino Galvani 2018-06-25 15:43:47 UTC
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

Comment 2 Beniamino Galvani 2018-06-27 08:23:02 UTC
Pushed branch bg/parse-var-attr-ref-rh1594887 for review.

Comment 3 Thomas Haller 2018-07-01 09:08:45 UTC
> 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.

Comment 5 Thomas Haller 2018-09-19 12:31:34 UTC
This missed rhel-7.6.
It also missed rhel-8.0 (aside the bits from comment 4).

Comment 6 Beniamino Galvani 2018-11-22 17:34:55 UTC
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.