Skip to content

Port to GLnxTmpfile#958

Closed
cgwalters wants to merge 4 commits intoostreedev:masterfrom
cgwalters:commit-tmpfile
Closed

Port to GLnxTmpfile#958
cgwalters wants to merge 4 commits intoostreedev:masterfrom
cgwalters:commit-tmpfile

Conversation

@cgwalters
Copy link
Copy Markdown
Member

@cgwalters cgwalters commented Jun 24, 2017

There's lots of mechanically replacing OtTmpFile with GLnxTmpfile;
the biggest changes are in the commit path. Symlink commits are now
very clearly separated from regular files. Symlinks are OtCleanupUnlinkat,
and regular files are GLnxTmpfile.

The commit codepath separates those as _ostree_repo_commit_path_final() and
_ostree_repo_commit_tmpf_final(). A nice aspect of all of this is that they
both consume the temporary on success. This avoids an extra spurious
unlink() call.

One of the biggest bits of code motion is in commit_loose_regfile_object(),
which no longer needs to care about symlinks. For the most parth though it's
just removing conditionals.

Update submodule: libglnx

@cgwalters
Copy link
Copy Markdown
Member Author

An updated version of #861

@cgwalters cgwalters added the WIP label Jun 24, 2017
@cgwalters
Copy link
Copy Markdown
Member Author

Prep in: #957

@rh-atomic-bot
Copy link
Copy Markdown

☔ The latest upstream changes (presumably da0791f) made this pull request unmergeable. Please resolve the merge conflicts.

There's lots of mechanically replacing `OtTmpFile` with `GLnxTmpfile`;
the biggest changes are in the commit path.  Symlink commits are now
very clearly separated from regular files.  Symlinks are `OtCleanupUnlinkat`,
and regular files are `GLnxTmpfile`.

The commit codepath separates those as `_ostree_repo_commit_path_final()` and
`_ostree_repo_commit_tmpf_final()`. A nice aspect of all of this is that they
both *consume* the temporary on success. This avoids an extra spurious
`unlink()` call.

One of the biggest bits of code motion is in `commit_loose_regfile_object()`,
which no longer needs to care about symlinks. For the most parth though it's
just removing conditionals.

Update submodule: libglnx
@cgwalters
Copy link
Copy Markdown
Member Author

Rebased 🏄‍♂️ and fixed up commit message. No longer WIP.

@cgwalters
Copy link
Copy Markdown
Member Author

(Though for some reason at least here github is failing to rerender this PR update)

@cgwalters cgwalters removed the WIP label Jun 26, 2017
@cgwalters cgwalters changed the title WIP: Commit tmpfile Port to GLnxTmpfile Jun 26, 2017
Copy link
Copy Markdown
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Looks sane. Just one minor nit.

{
g_assert (_ostree_repo_mode_is_bare (repo_mode));
/* Note: This will not be hit for bare-user mode because its converted to a
regular file and take the branch above */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: this comment needs to be updated.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done ⬇️

@jlebon
Copy link
Copy Markdown
Member

jlebon commented Jun 27, 2017

Hmm, we should set up testing against a CentOS kernel.

@jlebon
Copy link
Copy Markdown
Member

jlebon commented Jun 27, 2017

Let's do #968 first?

@jlebon
Copy link
Copy Markdown
Member

jlebon commented Jun 27, 2017

@rh-atomic-bot r+ 912adc6

@rh-atomic-bot
Copy link
Copy Markdown

⌛ Testing commit 912adc6 with merge 5776d5d...

@rh-atomic-bot
Copy link
Copy Markdown

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 5776d5d 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