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

Add stub for new libglnx tmpfile API, port simpler callers to it #871

Closed
wants to merge 2 commits into from

Conversation

cgwalters
Copy link
Member

It's hard right now to do a full port to the new libglnx tmpfile
API since there are complex cases in the commit path which deal
with symlinks as well.

Let's make things more gradual by introducing the important part (struct with
autocleanup) here in libotutil, port what we can. This will make a future
complete port easier.

@cgwalters
Copy link
Member Author

This can come after #865

@jlebon
Copy link
Member

jlebon commented May 17, 2017

So looking at the codebase, it seems like there's only 3 other instances where that API is used, right? It feels like it would be simpler than just do it all in one PR (even if it takes time) rather than carry a wrapper API and freeze libglnx? Or do you feel like those other 3 instances are especially complex?

@cgwalters
Copy link
Member Author

The problem is the commit codepath which uses temp_filename for symlinks, among other things. And the commit code is...way too complex for its own good. It needs its own series of preps I think.

@@ -25,6 +25,54 @@
#include <sys/xattr.h>
#include <gio/gunixinputstream.h>

/* Stub out old libglnx API to new one */
Copy link
Member

Choose a reason for hiding this comment

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

Can we expand on this a bit? E.g.

Wrapper around libglnx API, which doesn't clean up properly.
This was fixed in https://github.com/GNOME/libglnx/commit/9929adc, though
it's API breaking. Carry the fix here until we're ready to fully port.

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did that, also rebased on the new version of the API and cleaned up. ⬇️

cgwalters added 2 commits May 19, 2017 16:19
It's hard right now to do a full port to the new libglnx tmpfile
API since there are complex cases in the commit path which deal
with symlinks as well.

Let's make things more gradual by introducing the important part (struct with
autocleanup) here in libotutil, port what we can. This will make a future
complete port easier.
@cgwalters cgwalters force-pushed the glnx-tmpfile-prep branch from c979a06 to e4b8282 Compare May 19, 2017 20:26
@jlebon
Copy link
Member

jlebon commented May 23, 2017

@rh-atomic-bot r+ e4b8282

@rh-atomic-bot
Copy link

⌛ Testing commit e4b8282 with merge e99777e...

rh-atomic-bot pushed a commit that referenced this pull request May 23, 2017
It's hard right now to do a full port to the new libglnx tmpfile
API since there are complex cases in the commit path which deal
with symlinks as well.

Let's make things more gradual by introducing the important part (struct with
autocleanup) here in libotutil, port what we can. This will make a future
complete port easier.

Closes: #871
Approved by: jlebon
@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing e99777e to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants