GNOME Bugzilla – Bug 757442
Fix permissions and error handling for GIR output files
Last modified: 2015-11-01 23:28:47 UTC
There are a couple of issues with how the output file of g-ir-scanner is renamed from the temporary work file. The patches change the behavior to be more sensible.
Created attachment 314582 [details] [review] Don't lose output silently when overwrite of the output file is denied If the move resulted in EPERM, discard the temporary output file, but raise the error so that the tool exits with a nonzero status.
Created attachment 314583 [details] [review] g-ir-scanner: Set sensible permissions on the output file If the file is new, set the permissions to 0644. If the file with the name specified as --output exists, copy its metadata onto the temporary output file.
Review of attachment 314582 [details] [review]: Why only unlink on EPERM? Looks like the code came from dfff733b154abf1e85d447d2da389b34396fe005 - no rationale there. I'm fine if you keep it as is or delete the if (eperm) bit.
Review of attachment 314583 [details] [review]: I'd imagine there's a more idiomatic way to do this, like the equivalent of `g_file_replace()`, but the code looks fine.
(In reply to Colin Walters from comment #3) > Review of attachment 314582 [details] [review] [review]: > > Why only unlink on EPERM? Looks like the code came from > dfff733b154abf1e85d447d2da389b34396fe005 - no rationale there. My guess is that EPERM is an expected outcome -- we know exactly what's wrong, so the temporary file is unnecessary for post-error analysis. In case of an unexpected exception, the partial output may give some clues.
(In reply to Colin Walters from comment #4) > > I'd imagine there's a more idiomatic way to do this, like the equivalent of > `g_file_replace()`, but the code looks fine. I have implemented it with Python context manager protocol elsewhere: https://github.com/gi-rust/grust-gen/blob/master/grust/output.py#L25 In case of giscanner modules, however, this is the only place where a file created by tempfile.mkstemp() ends up being moved to become the eventual output file. If more reuse is needed, feel free to take code from my module. Besides, the branch for --reparse-validate=no just opens the output file for overwriting. That might be good to switch to flash-cutover as well.
Comment on attachment 314582 [details] [review] Don't lose output silently when overwrite of the output file is denied Pushed as commit 1754b3c
Comment on attachment 314583 [details] [review] g-ir-scanner: Set sensible permissions on the output file Pushed as commit 40d8236