From 994bdf29c67689a1fd4e54300a266db3a8f9ffa8 Mon Sep 17 00:00:00 2001 From: Kevin Day Date: Tue, 19 Apr 2022 22:10:20 -0500 Subject: [PATCH] Bugfix: Fixes for f_file exposed by unit tests. Restructure private_f_file_close() to be more consistent with the parameter ordering as is done with the rest of the project. Have f_file_stream_close() not handle flush errors so that close is only to be called once. The design of fclose() and close() state that even on error the descriptors are freed. This means that file close doesn't fail, it always succeeds. But the errors need to be propogated. Ignore flush and always call close so that the behavior of never calling close twice can be guaranteed. The private_f_file_close() will still This also means that the stream and descriptor are to always be reset on close. Return values need to be updated for several functions. Rename f_file_stream_descriptor() to f_file_stream_open_descriptor() to make it clear that this is an open command. --- level_0/f_file/c/file.c | 68 ++++++++++++++++++++++++++--------------- level_0/f_file/c/file.h | 27 ++++++++++------ level_0/f_file/c/private-file.c | 37 +++++++++++----------- level_0/f_file/c/private-file.h | 16 +++++----- 4 files changed, 87 insertions(+), 61 deletions(-) diff --git a/level_0/f_file/c/file.c b/level_0/f_file/c/file.c index ecdd1ee..3c08186 100644 --- a/level_0/f_file/c/file.c +++ b/level_0/f_file/c/file.c @@ -131,7 +131,7 @@ extern "C" { if (!id) return F_status_set_error(F_parameter); #endif // _di_level_0_parameter_checking_ - return private_f_file_close(id, F_false); + return private_f_file_close(F_false, id); } #endif // _di_f_file_close_ @@ -141,7 +141,7 @@ extern "C" { if (!id) return F_status_set_error(F_parameter); #endif // _di_level_0_parameter_checking_ - return private_f_file_close(id, F_true); + return private_f_file_close(F_true, id); } #endif // _di_f_file_close_flush_ @@ -2025,15 +2025,24 @@ extern "C" { #endif // _di_f_file_stat_by_id_ #ifndef _di_f_file_stream_close_ - f_status_t f_file_stream_close(const bool complete, f_file_t * const file) { + f_status_t f_file_stream_close(const bool flush, f_file_t * const file) { #ifndef _di_level_0_parameter_checking_ if (!file) return F_status_set_error(F_parameter); #endif // _di_level_0_parameter_checking_ if (file->stream) { + if (flush) { + fflush(file->stream); + } + if (fclose(file->stream) == EOF) { + + // According to man pages, further access to a stream on error results in undefined behavior. + file->stream = 0; + file->id = -1; + if (errno == EACCES) return F_status_set_error(F_access_denied); - if (errno == EAGAIN) return F_status_set_error(F_prohibited); + if (errno == EAGAIN || errno == EWOULDBLOCK) return F_status_set_error(F_block); if (errno == EBADF) return F_status_set_error(F_file_descriptor); if (errno == EFBIG) return F_status_set_error(F_file_overflow); if (errno == EDEADLK) return F_status_set_error(F_deadlock); @@ -2049,66 +2058,69 @@ extern "C" { if (errno == ENOTDIR) return F_status_set_error(F_file_type_not_directory); if (errno == EPERM) return F_status_set_error(F_prohibited); if (errno == EPIPE) return F_status_set_error(F_pipe_not); - if (errno == EWOULDBLOCK) return F_status_set_error(F_block); - return F_status_set_error(F_failure); + return F_status_set_error(F_file_close); } file->stream = 0; - - // File stream will result in the file descriptor being invalid because it is already closed. - if (complete) { - file->id = -1; - } + file->id = -1; return F_none; } - if (complete) { - return private_f_file_close(&file->id, F_true); - } + const f_status_t status = private_f_file_close(flush, &file->id); - return F_none; + file->stream = 0; + file->id = -1; + + return status; } #endif // _di_f_file_stream_close_ -#ifndef _di_f_file_stream_descriptor_ - f_status_t f_file_stream_descriptor(const f_string_static_t mode, f_file_t * const file) { +#ifndef _di_f_file_stream_open_descriptor_ + f_status_t f_file_stream_open_descriptor(const f_string_static_t mode, f_file_t * const file) { #ifndef _di_level_0_parameter_checking_ if (!file) return F_status_set_error(F_parameter); #endif // _di_level_0_parameter_checking_ if (file->id == -1) { - return F_status_set_error(F_file_closed); + return F_status_set_error(F_file_descriptor); } - if (mode.string) { - file->stream = fdopen(file->id, mode.string); + if (mode.used) { + file->stream = fdopen(file->id, private_f_file_stream_open_mode_determine(file->flag)); } else { - file->stream = fdopen(file->id, private_f_file_stream_open_mode_determine(file->flag)); + file->stream = fdopen(file->id, mode.string); } if (!file->stream) { if (errno == EACCES) return F_status_set_error(F_access_denied); - if (errno == EAGAIN) return F_status_set_error(F_prohibited); + if (errno == EAGAIN || errno == EWOULDBLOCK) return F_status_set_error(F_block); if (errno == EBADF) return F_status_set_error(F_file_descriptor); + if (errno == EFBIG) return F_status_set_error(F_file_overflow); if (errno == EDEADLK) return F_status_set_error(F_deadlock); + if (errno == EDESTADDRREQ) return F_status_set_error(F_socket_not); + if (errno == EDQUOT) return F_status_set_error(F_space_not); if (errno == EFAULT) return F_status_set_error(F_buffer); if (errno == EINTR) return F_status_set_error(F_interrupt); if (errno == EINVAL) return F_status_set_error(F_parameter); + if (errno == EIO) return F_status_set_error(F_input_output); if (errno == EMFILE) return F_status_set_error(F_file_descriptor_max); + if (errno == ENODEV) return F_status_set_error(F_device_not); if (errno == ENOLCK) return F_status_set_error(F_lock); + if (errno == ENOMEM) return F_status_set_error(F_memory_not); + if (errno == ENOSPC) return F_status_set_error(F_space_not); if (errno == ENOTDIR) return F_status_set_error(F_file_type_not_directory); if (errno == EPERM) return F_status_set_error(F_prohibited); + if (errno == EPIPE) return F_status_set_error(F_pipe_not); return F_status_set_error(F_failure); } return F_none; } -#endif // _di_f_file_stream_descriptor_ - +#endif // _di_f_file_stream_open_descriptor_ #ifndef _di_f_file_stream_open_ f_status_t f_file_stream_open(const f_string_static_t path, const f_string_static_t mode, f_file_t * const file) { @@ -2389,16 +2401,22 @@ extern "C" { if (!result) { if (errno == EACCES) return F_status_set_error(F_access_denied); - if (errno == EAGAIN) return F_status_set_error(F_prohibited); + if (errno == EAGAIN || errno == EWOULDBLOCK) return F_status_set_error(F_block); if (errno == EBADF) return F_status_set_error(F_file_descriptor); + if (errno == EFBIG) return F_status_set_error(F_file_overflow); if (errno == EDEADLK) return F_status_set_error(F_deadlock); + if (errno == EDESTADDRREQ) return F_status_set_error(F_socket_not); + if (errno == EDQUOT) return F_status_set_error(F_space_not); if (errno == EFAULT) return F_status_set_error(F_buffer); if (errno == EINTR) return F_status_set_error(F_interrupt); if (errno == EINVAL) return F_status_set_error(F_parameter); + if (errno == EIO) return F_status_set_error(F_input_output); if (errno == EMFILE) return F_status_set_error(F_file_descriptor_max); if (errno == ENOLCK) return F_status_set_error(F_lock); + if (errno == ENOSPC) return F_status_set_error(F_space_not); if (errno == ENOTDIR) return F_status_set_error(F_file_type_not_directory); if (errno == EPERM) return F_status_set_error(F_prohibited); + if (errno == EPIPE) return F_status_set_error(F_pipe_not); return F_status_set_error(F_failure); } diff --git a/level_0/f_file/c/file.h b/level_0/f_file/c/file.h index 7bdcc09..f5b08a3 100644 --- a/level_0/f_file/c/file.h +++ b/level_0/f_file/c/file.h @@ -2044,12 +2044,15 @@ extern "C" { /** * Close an open file stream. * - * @param complete - * If TRUE, will close the file descriptor as well, setting file.id is reset to -1, on success. - * If FALSE, will do nothing in regards to the file descriptor. + * @param flush + * If TRUE, will explicitly flush all unwritten data in any buffers to the file. + * If FALSE, will not explicitly flush unwritten data. + * + * If TRUE and flush fails, then this will still attempt to close the stream. * @param file * The file information. - * The file.stream is set to 0, on success. + * The file.stream is set to 0, on success or on failure. + * The file.id is set to 0, on success or on failure. * * @return * F_none is returned on success. @@ -2070,15 +2073,15 @@ extern "C" { * F_lock (with error bit) if failed to lock, such as lock table is full or too many open segments. * F_parameter (with error bit) if a parameter is invalid. * F_pipe_not (with error bit) if the stream is a pipe or a socket but the pipe or socket is already closed. - * F_prohibited (with error bit) if file system does not allow for making changes. * F_space_not (with error bit) if file system is out of space (or file system quota is reached). * F_socket_not (with error bit) if the datagram socket in which a peer has not been set (for socket related streams). * * @see close() * @see fclose() + * @see fflush() */ #ifndef _di_f_file_stream_close_ - extern f_status_t f_file_stream_close(const bool complete, f_file_t * const file); + extern f_status_t f_file_stream_close(const bool flush, f_file_t * const file); #endif // _di_f_file_stream_close_ /** @@ -2097,22 +2100,26 @@ extern "C" { * F_none is returned on success. * * F_access_denied (with error bit) on access denied. + * F_block (with error bit) if the action would block and non-blocking is set on the stream. * F_buffer (with error bit) if the buffer is invalid. * F_deadlock (with error bit) if operation would cause a deadlock. - * F_file_closed (with error bit) if file is not open. * F_file_descriptor (with error bit) if file descriptor is invalid. * F_file_descriptor_max (with error bit) if max file descriptors is reached. + * F_file_overflow (with error bit) if the write exceeds some implementation defined maximum file size. * F_file_type_not_directory (with error bit) if F_NOTIFY was specified and file.id is not a directory. * F_interrupt (with error bit) when program received an interrupt signal, halting operation. * F_lock (with error bit) if failed to lock, such as lock table is full or too many open segments. * F_parameter (with error bit) if a parameter is invalid. + * F_pipe_not (with error bit) if the stream is a pipe or a socket but the pipe or socket is already closed. * F_prohibited (with error bit) if file system does not allow for making changes. + * F_socket_not (with error bit) if socket is not connected. + * F_space_not (with error bit) if the file system is out of space (or file system quota is reached). * * @see fdopen() */ -#ifndef _di_f_file_stream_descriptor_ - extern f_status_t f_file_stream_descriptor(const f_string_static_t mode, f_file_t * const file); -#endif // _di_f_file_stream_descriptor_ +#ifndef _di_f_file_stream_open_descriptor_ + extern f_status_t f_file_stream_open_descriptor(const f_string_static_t mode, f_file_t * const file); +#endif // _di_f_file_stream_open_descriptor_ /** * Open a file stream. diff --git a/level_0/f_file/c/private-file.c b/level_0/f_file/c/private-file.c index 00e2183..5f69be9 100644 --- a/level_0/f_file/c/private-file.c +++ b/level_0/f_file/c/private-file.c @@ -6,14 +6,14 @@ extern "C" { #endif #if !defined(_di_f_file_close_) || !defined(_di_f_file_copy_) || !defined(_di_f_file_stream_close_) - f_status_t private_f_file_close(int * const id, const bool flush) { + f_status_t private_f_file_close(const bool flush, int * const id) { if (*id == -1) { return F_none; } - if (flush && F_status_is_error(private_f_file_flush(*id))) { - return F_status_set_error(F_file_synchronize); + if (flush) { + private_f_file_flush(*id); } if (close(*id) < 0) { @@ -53,7 +53,7 @@ extern "C" { status = private_f_file_open(destination, 0, &file_destination); if (F_status_is_error(status)) { - private_f_file_close(&file_source.id, F_true); + private_f_file_close(F_true, &file_source.id); return status; } @@ -69,15 +69,15 @@ extern "C" { size_write = write(file_destination.id, buffer, size_read); if (size_write < 0 || size_write != size_read) { - private_f_file_close(&file_destination.id, F_true); - private_f_file_close(&file_source.id, F_true); + private_f_file_close(F_true, &file_destination.id); + private_f_file_close(F_true, &file_source.id); return F_status_set_error(F_file_write); } } // while - private_f_file_close(&file_destination.id, F_true); - private_f_file_close(&file_source.id, F_true); + private_f_file_close(F_true, &file_destination.id); + private_f_file_close(F_true, &file_source.id); if (size_read < 0) { return F_status_set_error(F_file_read); @@ -102,7 +102,7 @@ extern "C" { status = private_f_file_open_at(at_id, destination, 0, &file_destination); if (F_status_is_error(status)) { - private_f_file_close(&file_source.id, F_true); + private_f_file_close(F_true, &file_source.id); return status; } @@ -118,15 +118,15 @@ extern "C" { size_write = write(file_destination.id, buffer, size_read); if (size_write < 0 || size_write != size_read) { - private_f_file_close(&file_destination.id, F_true); - private_f_file_close(&file_source.id, F_true); + private_f_file_close(F_true, &file_destination.id); + private_f_file_close(F_true, &file_source.id); return F_status_set_error(F_file_write); } } // while - private_f_file_close(&file_destination.id, F_true); - private_f_file_close(&file_source.id, F_true); + private_f_file_close(F_true, &file_destination.id); + private_f_file_close(F_true, &file_source.id); if (size_read < 0) { return F_status_set_error(F_file_read); @@ -150,7 +150,7 @@ extern "C" { const f_status_t status = private_f_file_open(path, mode, &file); if (file.id != -1) { - return private_f_file_close(&file.id, F_true); + return private_f_file_close(F_true, &file.id); } return status; @@ -171,7 +171,7 @@ extern "C" { const f_status_t status = private_f_file_open_at(at_id, path, mode, &file); if (file.id != -1) { - return private_f_file_close(&file.id, F_true); + return private_f_file_close(F_true, &file.id); } return status; @@ -333,11 +333,12 @@ extern "C" { if (fsync(id) < 0) { if (errno == EBADF) return F_status_set_error(F_file_descriptor); if (errno == EDQUOT) return F_status_set_error(F_filesystem_quota_block); + if (errno == EINVAL) return F_status_set_error(F_supported_not); if (errno == EIO) return F_status_set_error(F_input_output); if (errno == ENOSPC) return F_status_set_error(F_space_not); if (errno == EROFS) return F_status_set_error(F_supported_not); - return F_status_set_error(F_failure); + return F_status_set_error(F_file_synchronize); } return F_none; @@ -734,7 +735,7 @@ extern "C" { } #endif // !defined(_di_f_file_stat_by_id_) || !defined(_di_f_file_size_by_id_) -#if !defined(_di_f_file_stream_descriptor_) || !defined(_di_f_file_stream_open_) || !defined(_di_f_file_stream_reopen_) +#if !defined(_di_f_file_stream_open_descriptor_) || !defined(_di_f_file_stream_open_) || !defined(_di_f_file_stream_reopen_) const f_string_t private_f_file_stream_open_mode_determine(const int flag) { if (flag & F_file_flag_read_write_d) { @@ -761,7 +762,7 @@ extern "C" { // Failsafe to read only. return f_file_open_mode_read_s.string; } -#endif // !defined(_di_f_file_stream_descriptor_) || !defined(_di_f_file_stream_open_) || !defined(_di_f_file_stream_reopen_) +#endif // !defined(_di_f_file_stream_open_descriptor_) || !defined(_di_f_file_stream_open_) || !defined(_di_f_file_stream_reopen_) #if !defined(f_file_stream_write) || !defined(_di_f_file_stream_write_block_) || !defined(f_file_stream_write_until) || !defined(f_file_stream_write_range) f_status_t private_f_file_stream_write_until(const f_file_t file, const f_string_static_t buffer, const f_array_length_t total, f_array_length_t * const written) { diff --git a/level_0/f_file/c/private-file.h b/level_0/f_file/c/private-file.h index 2d798cd..ae6edbe 100644 --- a/level_0/f_file/c/private-file.h +++ b/level_0/f_file/c/private-file.h @@ -20,12 +20,12 @@ extern "C" { * * Intended to be shared to each of the different implementation variations. * - * @param id - * The file descriptor. - * The value gets set to -1. * @param flush * If TRUE, then perform flush before closing. * If FALSE, then do not perform flush before closing. + * @param id + * The file descriptor. + * The value gets set to -1. * * @return * F_none on success. @@ -44,7 +44,7 @@ extern "C" { * @see f_file_stream_close() */ #if !defined(_di_f_file_close_) || !defined(_di_f_file_copy_) || !defined(_di_f_file_stream_close_) - extern f_status_t private_f_file_close(int * const id, const bool flush) F_attribute_visibility_internal_d; + extern f_status_t private_f_file_close(const bool flush, int * const id) F_attribute_visibility_internal_d; #endif // !defined(_di_f_file_close_) || !defined(_di_f_file_copy_) || !defined(_di_f_file_stream_close_) /** @@ -427,7 +427,7 @@ extern "C" { * F_parameter (with error bit) if a parameter is invalid. * F_space_not (with error bit) if file system is out of space (or file system quota is reached). * F_supported_not (with error bit) if the file system or file type does not support flushing. - * F_failure (with error bit) on any other failure. + * F_file_synchronize (with error bit) on any other failure. * * @see f_file_close() * @see f_file_copy() @@ -902,13 +902,13 @@ extern "C" { * @return * A string representing the file mode for use in fopen(), fdopen(), or freopen(). * - * @see f_file_stream_descriptor() + * @see f_file_stream_open_descriptor() * @see f_file_stream_open() * @see f_file_stream_reopen() */ -#if !defined(_di_f_file_stream_descriptor_) || !defined(_di_f_file_stream_open_) || !defined(_di_f_file_stream_reopen_) +#if !defined(_di_f_file_stream_open_descriptor_) || !defined(_di_f_file_stream_open_) || !defined(_di_f_file_stream_reopen_) extern const f_string_t private_f_file_stream_open_mode_determine(const int flag) F_attribute_visibility_internal_d; -#endif // !defined(_di_f_file_stream_descriptor_) || !defined(_di_f_file_stream_open_) || !defined(_di_f_file_stream_reopen_) +#endif // !defined(_di_f_file_stream_open_descriptor_) || !defined(_di_f_file_stream_open_) || !defined(_di_f_file_stream_reopen_) /** * Private implementation of f_file_stream_write_until(). -- 1.8.3.1