Opened 14 years ago
Closed 14 years ago
#89 closed enhancement (fixed)
Patch to clean up vuurmuur_fopen
Reported by: | Matthijs Kooijman | Owned by: | Victor Julien |
---|---|---|---|
Priority: | minor | Milestone: | undecided |
Component: | libvuurmuur | Version: | |
Keywords: | patch | Cc: |
Description
The attached patch cleans up vuurmuur_fopen
a bit. It removes all checks from the function and instead calls stat_ok
do do these checks. This has the following functional changes:
vuurmuur_fopen
now only works for regular files (non fifo's, character devices, etc.). This shouldn't be a problem, config files should be regular files anyway.vuurmuur_fopen
now only works for existing files. This is becausestat_ok
fails when a stat call fails, without special case for a "File not found" error. The originalvuurmuur_fopen
function skipped all checks whenlstat
returned an error (which might have been wrong, since an error does not always mean "File not found").
However, I have checked all uses of thevuurmuur_fopen
function, and all of them use it only on existing files (add_textdir
uses plainf
open
to create new files.
The patch is against svn r240.
Attachments (5)
Change History (11)
by , 14 years ago
Attachment: | fopen-use-statok.diff added |
---|
comment:1 by , 14 years ago
There is one more change this patch makes: It adds a {{{debuglvl}} parameter to the vuurmuur_fopen and rules_file_open functions, since that needs to be passed to statok. All users within vuurmuur have been updated, but this might be an issue if these functions are part of the external API.
comment:2 by , 14 years ago
Even though the vuurmuur_fopen function is not used to create files atm, please adapt your patch so that it's still possible for future use.
Patch looks good otherwise.
comment:3 by , 14 years ago
So far I've tried to prevent this, because it would require stat_ok as well. But, I'll give it a try then (because I agree it would be more elegant). However, this would require modifying stat_ok to (optionally) not fail when the file does not exist. This should probably be another (boolean) parameter, "must_exist", which triggers stat_ok to fail if the file does not exist. Would this be ok?
by , 14 years ago
Attachment: | 00-statok-mustexist added |
---|
Add a mustexist parameter to stat_ok, allowing stat_ok to succeed when a file does not exist
by , 14 years ago
Attachment: | 01-fopen-use-statok added |
---|
Make vuurmuur_fopen use stat_ok for its checks. Lets vuurmuur_fopen work even for non-existing files.
by , 14 years ago
Attachment: | 02-clean-statok-uses added |
---|
Do some cleanups enabled by the updated vuurmuur_fopen and stat_ok functions
by , 14 years ago
Attachment: | 03-clean-statok added |
---|
Remove some duplicate code from the statok function and improve the info messages
comment:4 by , 14 years ago
I've added a new series of patches. Patch 00 prepares stat_ok such that it will (optionally) not fail when the file does not exist. All existing calls to stat_ok have the STATOK_MUST_EXIST argument added, so no behaviour is changed (except for a difference in error message for file not found and other stat errors).
Patch 01 is an updated version of the original patch, which makes vuurmuur_fopen use statok while making it also work for non-existing files.
Patch 02 does some cleanup enabled by the above patches, while patch 03 does some cleanup of the stat_ok internals.
I've also looked at your suggestion on IRC: To see if calls to stat_ok could be prevented alltogether and stat_ok be made a static function in io.c. It seems there are different situations where stat_ok cannot be replaced by vuurmuur_fopen, so unless we want to skip the checks alltogether, I don't see how to implement this.
comment:6 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
The patch, against r240