From: Alan T. DeKok Date: Tue, 25 Jul 2017 13:50:14 +0000 (-0400) Subject: clean up exfile code. X-Git-Tag: release_3_0_16~203 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6cfa48dcd4e26687527fa3c3945922c567f2205e;p=thirdparty%2Ffreeradius-server.git clean up exfile code. If it's not locking, skip all of the management logic --- diff --git a/src/main/exfile.c b/src/main/exfile.c index c63c70ec8c5..245dee18a9e 100644 --- a/src/main/exfile.c +++ b/src/main/exfile.c @@ -102,6 +102,17 @@ exfile_t *exfile_init(TALLOC_CTX *ctx, uint32_t max_entries, uint32_t max_idle, ef = talloc_zero(ctx, exfile_t); if (!ef) return NULL; + ef->max_entries = max_entries; + ef->max_idle = max_idle; + ef->locking = locking; + + /* + * If we're not locking the files, just return the + * handle. Each call to exfile_open() will just open a + * new file descriptor. + */ + if (!locking) return ef; + ef->entries = talloc_zero_array(ef, exfile_entry_t, max_entries); if (!ef->entries) { talloc_free(ef); @@ -115,10 +126,6 @@ exfile_t *exfile_init(TALLOC_CTX *ctx, uint32_t max_entries, uint32_t max_idle, } #endif - ef->max_entries = max_entries; - ef->max_idle = max_idle; - ef->locking = locking; - talloc_set_destructor(ef, _exfile_free); return ef; @@ -135,6 +142,61 @@ static void exfile_cleanup_entry(exfile_entry_t *entry) } +/* + * Try to open the file. It it doesn't exist, try to + * create it's parent directories. + */ +static int exfile_open_mkdir(exfile_t *ef, char const *filename, mode_t permissions) +{ + int fd; + + fd = open(filename, O_RDWR | O_CREAT, permissions); + if (fd < 0) { + mode_t dirperm; + char *p, *dir; + + /* + * Maybe the directory doesn't exist. Try to + * create it. + */ + dir = talloc_strdup(ef, filename); + if (!dir) return -1; + p = strrchr(dir, FR_DIR_SEP); + if (!p) { + fr_strerror_printf("No '/' in '%s'", filename); + return -1; + } + *p = '\0'; + + /* + * Ensure that the 'x' bit is set, so that we can + * read the directory. + */ + dirperm = permissions; + if ((dirperm & 0600) != 0) dirperm |= 0100; + if ((dirperm & 0060) != 0) dirperm |= 0010; + if ((dirperm & 0006) != 0) dirperm |= 0001; + + if (rad_mkdir(dir, dirperm, -1, -1) < 0) { + fr_strerror_printf("Failed to create directory %s: %s", + dir, strerror(errno)); + talloc_free(dir); + return -1; + } + talloc_free(dir); + + fd = open(filename, O_RDWR | O_CREAT, permissions); + if (fd < 0) { + fr_strerror_printf("Failed to open file %s: %s", + filename, strerror(errno)); + return -1; + } + } + + return fd; +} + + /** Open a new log file, or maybe an existing one. * * When multithreaded, the FD is locked via a mutex. This way we're @@ -150,12 +212,29 @@ int exfile_open(exfile_t *ef, char const *filename, mode_t permissions, bool app { int i, found, tries, unused, oldest; uint32_t hash; - time_t now = time(NULL); + time_t now; struct stat st; if (!ef || !filename) return -1; + /* + * No locking: just return a new FD. + */ + if (!ef->locking) { + found = exfile_open_mkdir(ef, filename, permissions); + if (found < 0) return -1; + + if (append) (void) lseek(found, 0, SEEK_END); + + return found; + } + + /* + * It's faster to do hash comparisons of a string than + * full string comparisons. + */ hash = fr_hash_string(filename); + now = time(NULL); PTHREAD_MUTEX_LOCK(&ef->mutex); @@ -239,48 +318,8 @@ reopen: /* * Open the file and try to lock it. */ - ef->entries[i].fd = open(filename, O_RDWR | O_APPEND | O_CREAT, permissions); - if (ef->entries[i].fd < 0) { - mode_t dirperm; - char *p, *dir; - - /* - * Maybe the directory doesn't exist. Try to - * create it. - */ - dir = talloc_strdup(ef, filename); - if (!dir) goto error; - p = strrchr(dir, FR_DIR_SEP); - if (!p) { - fr_strerror_printf("No '/' in '%s'", filename); - goto error; - } - *p = '\0'; - - /* - * Ensure that the 'x' bit is set, so that we can - * read the directory. - */ - dirperm = permissions; - if ((dirperm & 0600) != 0) dirperm |= 0100; - if ((dirperm & 0060) != 0) dirperm |= 0010; - if ((dirperm & 0006) != 0) dirperm |= 0001; - - if (rad_mkdir(dir, dirperm, -1, -1) < 0) { - fr_strerror_printf("Failed to create directory %s: %s", - dir, strerror(errno)); - talloc_free(dir); - goto error; - } - talloc_free(dir); - - ef->entries[i].fd = open(filename, O_WRONLY | O_CREAT, permissions); - if (ef->entries[i].fd < 0) { - fr_strerror_printf("Failed to open file %s: %s", - filename, strerror(errno)); - goto error; - } /* else fall through to creating the rest of the entry */ - } /* else the file was already opened */ + ef->entries[i].fd = exfile_open_mkdir(ef, filename, permissions); + if (ef->entries[i].fd < 0) goto error; /* * Try to lock it. If we can't lock it, it's because @@ -288,50 +327,52 @@ reopen: * locked it. So, we close the current file, re-open it, * and try again. */ - if (ef->locking) { - /* - * Lock from the start of the file. - */ - if (lseek(ef->entries[i].fd, 0, SEEK_SET) < 0) { - fr_strerror_printf("Failed to seek in file %s: %s", filename, strerror(errno)); - error: - exfile_cleanup_entry(&ef->entries[i]); - PTHREAD_MUTEX_UNLOCK(&(ef->mutex)); - return -1; - } + /* + * Lock from the start of the file. It's the + * only point in the file which is guaranteed to + * exist, and to be consistent across all threads + * and processes. + */ + if (lseek(ef->entries[i].fd, 0, SEEK_SET) < 0) { + fr_strerror_printf("Failed to seek in file %s: %s", filename, strerror(errno)); - /* - * Busy-loop trying to lock the file. - */ - for (tries = 0; tries < MAX_TRY_LOCK; tries++) { - if (rad_lockfd_nonblock(ef->entries[i].fd, 0) >= 0) break; + error: + exfile_cleanup_entry(&ef->entries[i]); + PTHREAD_MUTEX_UNLOCK(&(ef->mutex)); + return -1; + } - if (errno != EAGAIN) { - fr_strerror_printf("Failed to lock file %s: %s", filename, strerror(errno)); - goto error; - } + /* + * Busy-loop trying to lock the file. + */ + for (tries = 0; tries < MAX_TRY_LOCK; tries++) { + if (rad_lockfd_nonblock(ef->entries[i].fd, 0) >= 0) break; - /* - * Close the file and re-open it. It may - * have been deleted. If it was deleted, - * then it should now be unlocked. - */ - close(ef->entries[i].fd); - ef->entries[i].fd = open(filename, O_WRONLY | O_CREAT, permissions); - if (ef->entries[i].fd < 0) { - fr_strerror_printf("Failed to open file %s: %s", - filename, strerror(errno)); - goto error; - } + if (errno != EAGAIN) { + fr_strerror_printf("Failed to lock file %s: %s", filename, strerror(errno)); + goto error; } - if (tries >= MAX_TRY_LOCK) { - fr_strerror_printf("Failed to lock file %s: too many tries", filename); + /* + * Close the file and re-open it. It may + * have been deleted. If it was deleted, + * then it should now be unlocked. + */ + close(ef->entries[i].fd); + ef->entries[i].fd = open(filename, O_WRONLY | O_CREAT, permissions); + if (ef->entries[i].fd < 0) { + fr_strerror_printf("Failed to open file %s: %s", + filename, strerror(errno)); goto error; } } + if (tries >= MAX_TRY_LOCK) { + fr_strerror_printf("Failed to lock file %s: too many tries", filename); + goto error; + } + /* * Maybe someone deleted the file while we were waiting * for the lock. If so, re-open it. @@ -347,8 +388,8 @@ reopen: } /* - * Seek to the end of the file before returning the FD to - * the caller. + * If we're appending, eek to the end of the file before + * returning the FD to the caller. */ if (append) lseek(ef->entries[i].fd, 0, SEEK_END); @@ -374,15 +415,21 @@ int exfile_close(exfile_t *ef, int fd) { uint32_t i; + /* + * No locking: just close the file. + */ + if (!ef->locking) { + close(fd); + return 0; + } + + /* + * Unlock the bytes that we had previously locked. + */ for (i = 0; i < ef->max_entries; i++) { - /* - * Unlock the bytes that we had previously locked. - */ if (ef->entries[i].fd == fd) { - if (ef->locking) { - (void) lseek(ef->entries[i].fd, 0, SEEK_SET); - (void) rad_unlockfd(ef->entries[i].fd, 0); - } + (void) lseek(ef->entries[i].fd, 0, SEEK_SET); + (void) rad_unlockfd(ef->entries[i].fd, 0); PTHREAD_MUTEX_UNLOCK(&(ef->mutex)); return 0;