lib: use tmpfile when writing out bpfilter state#412
lib: use tmpfile when writing out bpfilter state#412etsal wants to merge 1 commit intofacebook:mainfrom
Conversation
src/libbpfilter/helper.c
Outdated
| if (ret >= len) { | ||
| return bf_err_r(-ENAMETOOLONG, "tmpfile name too long (%lu bytes)", ret); | ||
| } |
There was a problem hiding this comment.
No braces for a single statement.
src/libbpfilter/helper.c
Outdated
| static int bf_tmp_name(char *tmpname, size_t len, const char *path) | ||
| { | ||
| char template[] = "/bpfilter_XXXXXX"; | ||
| char *tmp, *dir; |
There was a problem hiding this comment.
_cleanup_free_ char *tmp = NULL; for automatic cleanup.
src/libbpfilter/helper.c
Outdated
| } | ||
|
|
||
|
|
||
| static int bf_tmp_name(char *tmpname, size_t len, const char *path) |
There was a problem hiding this comment.
Static symbols are prefixed with _.
src/libbpfilter/helper.c
Outdated
| bf_assert(buf); | ||
|
|
||
| fd = open(path, O_TRUNC | O_CREAT | O_WRONLY, OPEN_MODE_644); | ||
| err = bf_tmp_name(tmpname, sizeof(tmpname), path); |
There was a problem hiding this comment.
bf_tmp_name does not abide by its contract, it creates a temporary file template name, not a temporary file name.
Additionally, it would be more convenient to have bf_get_tmpfile_fd(const char *base, char tmpfile_path[static PATH_MAX])
base: base path to create the tmp file into (after applyingdirname), so we can passpathdirectly. The final tmpfile will bedirname(base)/"bpfilter.tmp.XXXXXX"tmpfile_path: path to the new tmp file- Returns a file descriptor to the tmp file
src/libbpfilter/helper.c
Outdated
| fd = open(path, O_TRUNC | O_CREAT | O_WRONLY, OPEN_MODE_644); | ||
| err = bf_tmp_name(tmpname, sizeof(tmpname), path); | ||
| if (err < 0) | ||
| return bf_err_r(errno, "failed to get temporary file name"); |
There was a problem hiding this comment.
bf_tmp_name doesn't set errno, but returns -errno on error.
src/libbpfilter/helper.c
Outdated
| return 0; | ||
| } | ||
|
|
||
|
|
The bpfilter daemon updates persistent state by reopening the existing file, truncating it, then overwriting the state. If the daemon crashes during this process, when it restarts it will find either no data, or malformed data. Turn the state update atomic by instead writing the new persistent data to a temporary file, then moving it to the correct location.
etsal
force-pushed
the
atomic-file-update
branch
from
7d7d019 to
f423b83
Compare
|
New version addressing the feedback. |
qdeslandes
left a comment
qdeslandes
left a comment
There was a problem hiding this comment.
Few nits but otherwise should be OK. Also, please change the MR status when ready for a review: currently I see it as "changes requested", so I might overlook it.
| static int _bf_get_tmpfile_fd(const char *base, char tmpfile_path[PATH_MAX]) | ||
| { | ||
| char template[] = "/bpfilter.tmp.XXXXXX"; | ||
| _cleanup_free_ char *tmp; |
There was a problem hiding this comment.
tmp = NULL to ensure we don't try to cleanup a garbage point if we exit early.
| return 0; | ||
| } | ||
|
|
||
| static int _bf_get_tmpfile_fd(const char *base, char tmpfile_path[PATH_MAX]) |
|
|
||
| dir = dirname(tmp); | ||
|
|
||
| path_len = snprintf(tmpfile_path, PATH_MAX, "%s%s", dir, template); |
There was a problem hiding this comment.
No need to have a variable for template here: "%s/bpfilter.tmp.XXXXXX" should do it
2 participants
The bpfilter daemon updates persistent state by reopening the existing file, truncating it, then overwriting the state. If the daemon crashes during this process, when it restarts it will find either no data, or malformed data. Turn the state update atomic by instead writing the new persistent data to a temporary file, then moving it to the correct location.