Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use g_auto and G_VARIANT_BUILDER_INIT for all GVariantBuilders #1503

Merged
merged 8 commits into from
Nov 5, 2024

Conversation

swick
Copy link
Contributor

@swick swick commented Nov 1, 2024

  • Always initialize g_auto(GVariantBuilder)

  • Use g_auto and G_VARIANT_BUILDER_INIT for all GVariantBuilders

    One has to be very careful about the control flow and early exits
    without the g_auto which can easily result in memory leaks. I noticed at
    least 5 cases where the change does fix a leak, even though it might be
    a bit hard to trigger.

    This also makes sure that all g_auto usages are initialized correctly.

Copy link
Contributor

@whot whot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/remote-desktop.c Outdated Show resolved Hide resolved
Copy link
Contributor

@jsparber jsparber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but not sure what the backport does, would be good to explain it in the commit message.

src/glib-backports.h Outdated Show resolved Hide resolved
src/restore-token.c Outdated Show resolved Hide resolved
@swick
Copy link
Contributor Author

swick commented Nov 4, 2024

Dropped BUILDER_INIT_UNSET and its usage.

@jsparber
Copy link
Contributor

jsparber commented Nov 4, 2024

Dropped BUILDER_INIT_UNSET and its usage.

the commit is still there.

@swick
Copy link
Contributor Author

swick commented Nov 4, 2024

Woops, my bad.

@GeorgesStavracas GeorgesStavracas added this to the 1.20 milestone Nov 5, 2024
If it is missing, we just don't add it to the keyfile instead of running
into criticals. The XdpAppInfo impl can still add the right command
later on.
One has to be very careful about the control flow and early exits
without the g_auto which can easily result in memory leaks. I noticed at
least 5 cases where the change does fix a leak, even though it might be
a bit hard to trigger.

This also makes sure that all g_auto usages are initialized correctly.
@GeorgesStavracas GeorgesStavracas enabled auto-merge (rebase) November 5, 2024 14:42
@GeorgesStavracas GeorgesStavracas merged commit debd30e into flatpak:main Nov 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triaged
Development

Successfully merging this pull request may close these issues.

5 participants