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

Avoid creating temp files during manifest injection #31

Merged
merged 2 commits into from May 18, 2018

Conversation

mizdebsk
Copy link
Member

JDK seems to create temp files with umask 077, which results in wrong permissions on some installed files. See rhbz#1579236 for the original bug report.

This PR reverts to in-place manifest injection. JAR file is first opened, then unlinked, but i-node and file contents remain on disk. Then new file (new i-node) is created with the old name and new data written into it. This avoids creating any temporary files.

CC @mbooth101

@codecov-io
Copy link

codecov-io commented May 17, 2018

Codecov Report

Merging #31 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #31      +/-   ##
============================================
- Coverage     77.71%   77.71%   -0.01%     
  Complexity      253      253              
============================================
  Files           101      101              
  Lines          4470     4469       -1     
  Branches        716      716              
============================================
- Hits           3474     3473       -1     
  Misses          684      684              
  Partials        312      312
Impacted Files Coverage Δ Complexity Δ
...org/fedoraproject/xmvn/tools/install/JarUtils.java 79.12% <100%> (-0.23%) 0 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 917b610...c6773d9. Read the comment docs.

@mizdebsk
Copy link
Member Author

Sigh, I just realized this causes data loss (original JAR is removed) in case of some exception during JAR processing, so I probably should add a test case for this and further improve the code.

@mizdebsk mizdebsk merged commit ba99289 into master May 18, 2018
@mizdebsk mizdebsk deleted the mizdebsk/inplace-mf-injection branch May 18, 2018 09:26
@msimacek
Copy link
Contributor

Sigh, I just realized this causes data loss (original JAR is removed) in case of some exception during JAR processing, so I probably should add a test case for this and further improve the code.

That's the reason why I added the temporary file, because overwiting the JAR in place made debugging the original problem (jar corruption) quite painful. What about making the temporary file, but instead of using Files.move, copying the file content (after tmp file is done, open original with trunc and write the contents of tmp file into it, then delete tmp)?

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.

None yet

3 participants