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 because stat_ok fails when a stat call fails, without special case for a "File not found" error. The original vuurmuur_fopen function skipped all checks when lstat 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 the vuurmuur_fopen function, and all of them use it only on existing files (add_textdir uses plain fopen to create new files.

The patch is against svn r240.

Attachments (5)

fopen-use-statok.diff (8.3 KB ) - added by Matthijs Kooijman 14 years ago.
The patch, against r240
00-statok-mustexist (11.2 KB ) - added by Matthijs Kooijman 14 years ago.
Add a mustexist parameter to stat_ok, allowing stat_ok to succeed when a file does not exist
01-fopen-use-statok (8.3 KB ) - added by Matthijs Kooijman 14 years ago.
Make vuurmuur_fopen use stat_ok for its checks. Lets vuurmuur_fopen work even for non-existing files.
02-clean-statok-uses (3.7 KB ) - added by Matthijs Kooijman 14 years ago.
Do some cleanups enabled by the updated vuurmuur_fopen and stat_ok functions
03-clean-statok (2.3 KB ) - added by Matthijs Kooijman 14 years ago.
Remove some duplicate code from the statok function and improve the info messages

Download all attachments as: .zip

Change History (11)

by Matthijs Kooijman, 14 years ago

Attachment: fopen-use-statok.diff added

The patch, against r240

comment:1 by Matthijs Kooijman, 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 Victor Julien, 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 Matthijs Kooijman, 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 Matthijs Kooijman, 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 Matthijs Kooijman, 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 Matthijs Kooijman, 14 years ago

Attachment: 02-clean-statok-uses added

Do some cleanups enabled by the updated vuurmuur_fopen and stat_ok functions

by Matthijs Kooijman, 14 years ago

Attachment: 03-clean-statok added

Remove some duplicate code from the statok function and improve the info messages

comment:4 by Matthijs Kooijman, 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:5 by Victor Julien, 14 years ago

Thanks Matthijs. I applied the patches in changeset:244.

comment:6 by Victor Julien, 14 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.