From 11dda658d6f0c4470a54012df71be73b4e9a5f57 Mon Sep 17 00:00:00 2001 From: ZyX Date: Wed, 1 Jun 2016 22:57:52 +0300 Subject: [PATCH 01/13] file,os/fs,shada: Separate opening, closing, writing and reading files Moves low-level functions handling to os/fs.c. Adds file.c with a proxy interface. Target: while leaving syscalls handling is os.c (partially handled by libuv), add buffering for reading and writing to file.c. --- config/CMakeLists.txt | 1 + config/config.h.in | 1 + src/nvim/file.c | 164 +++++++++++++++++++++++++++++++ src/nvim/file.h | 59 +++++++++++ src/nvim/os/fs.c | 213 +++++++++++++++++++++++++++++---------- src/nvim/shada.c | 224 +++++++++++++++--------------------------- 6 files changed, 463 insertions(+), 199 deletions(-) create mode 100644 src/nvim/file.c create mode 100644 src/nvim/file.h diff --git a/config/CMakeLists.txt b/config/CMakeLists.txt index e794a8c5b9..e1e90c6a9d 100644 --- a/config/CMakeLists.txt +++ b/config/CMakeLists.txt @@ -33,6 +33,7 @@ check_function_exists(fseeko HAVE_FSEEKO) check_function_exists(getpwent HAVE_GETPWENT) check_function_exists(getpwnam HAVE_GETPWNAM) check_function_exists(getpwuid HAVE_GETPWUID) +check_function_exists(uv_translate_sys_error HAVE_UV_TRANSLATE_SYS_ERROR) if(Iconv_FOUND) set(HAVE_ICONV 1) diff --git a/config/config.h.in b/config/config.h.in index 867278de0d..7f16fd1928 100644 --- a/config/config.h.in +++ b/config/config.h.in @@ -30,6 +30,7 @@ #cmakedefine HAVE_PUTENV_S #cmakedefine HAVE_PWD_H #cmakedefine HAVE_READLINK +#cmakedefine HAVE_UV_TRANSLATE_SYS_ERROR // TODO: add proper cmake check // #define HAVE_SELINUX 1 #cmakedefine HAVE_SETENV diff --git a/src/nvim/file.c b/src/nvim/file.c new file mode 100644 index 0000000000..bc230ecf00 --- /dev/null +++ b/src/nvim/file.c @@ -0,0 +1,164 @@ +/// @file file.c +/// +/// Buffered reading/writing to a file. Unlike fileio.c this is not dealing with +/// Neovim stuctures for buffer, with autocommands, etc: just fopen/fread/fwrite +/// replacement. + +#include +#include +#include + +#include + +#include "nvim/file.h" +#include "nvim/memory.h" +#include "nvim/os/os.h" +#include "nvim/globals.h" + +#ifdef INCLUDE_GENERATED_DECLARATIONS +# include "file.c.generated.h" +#endif + +/// Open file +/// +/// @param[out] ret_fp Address where information needed for reading from or +/// writing to a file is saved +/// @param[in] fname File name to open. +/// @param[in] flags Flags, @see FileOpenFlags. +/// @param[in] mode Permissions for the newly created file (ignored if flags +/// does not have FILE_CREATE\*). +/// +/// @return Error code (@see os_strerror()) or 0. +int file_open(FileDescriptor *const ret_fp, const char *const fname, + const int flags, const int mode) + FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT +{ + int fd; + + fd = os_open(fname, flags, mode); + + if (fd < 0) { + return fd; + } + + ret_fp->fd = fd; + ret_fp->eof = false; + return 0; +} + +/// Like file_open(), but allocate and return ret_fp +/// +/// @param[out] error Error code, @see os_strerror(). Is set to zero on +/// success. +/// @param[in] fname File name to open. +/// @param[in] flags Flags, @see FileOpenFlags. +/// @param[in] mode Permissions for the newly created file (ignored if flags +/// does not have FILE_CREATE\*). +/// +/// @return [allocated] Opened file or NULL in case of error. +FileDescriptor *file_open_new(int *const error, const char *const fname, + const int flags, const int mode) + FUNC_ATTR_NONNULL_ALL FUNC_ATTR_MALLOC FUNC_ATTR_WARN_UNUSED_RESULT +{ + FileDescriptor *const fp = xmalloc(sizeof(*fp)); + if ((*error = file_open(fp, fname, flags, mode)) != 0) { + xfree(fp); + return NULL; + } + return fp; +} + +/// Close file +/// +/// @param[in,out] fp File to close. +/// +/// @return 0 or error code. +int file_close(FileDescriptor *const fp) FUNC_ATTR_NONNULL_ALL +{ + const int error = file_fsync(fp); + const int error2 = os_close(fp->fd); + if (error2 != 0) { + return error2; + } + return error; +} + +/// Close and free file obtained using file_open_new() +/// +/// @param[in,out] fp File to close. +/// +/// @return 0 or error code. +int file_free(FileDescriptor *const fp) FUNC_ATTR_NONNULL_ALL +{ + const int ret = file_close(fp); + xfree(fp); + return ret; +} + +/// Flush file modifications to disk +/// +/// @param[in,out] fp File to work with. +/// +/// @return 0 or error code. +int file_fsync(FileDescriptor *const fp) + FUNC_ATTR_NONNULL_ALL +{ + return os_fsync(fp->fd); +} + +/// Read from file +/// +/// @param[in,out] fp File to work with. +/// @param[out] ret_buf Buffer to read to. Must not be NULL. +/// @param[in] size Number of bytes to read. Buffer must have at least ret_buf +/// bytes. +/// +/// @return error_code (< 0) or number of bytes read. +ptrdiff_t file_read(FileDescriptor *const fp, char *const ret_buf, + const size_t size) + FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT +{ + return os_read(fp->fd, &fp->eof, ret_buf, size); +} + +/// Write to a file +/// +/// @param[in] fd File descriptor to write to. +/// @param[in] buf Data to write. May be NULL if size is zero. +/// @param[in] size Amount of bytes to write. +/// +/// @return Number of bytes written or libuv error code (< 0). +ptrdiff_t file_write(FileDescriptor *const fp, const char *const buf, + const size_t size) + FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ARG(1) +{ + return os_write(fp->fd, buf, size); +} + +/// Buffer used for skipping. Its contents is undefined and should never be +/// used. +static char skipbuf[IOSIZE]; + +/// Skip some bytes +/// +/// This is like `fseek(fp, size, SEEK_CUR)`, but actual implementation simply +/// reads to a buffer and discards the result. +ptrdiff_t file_skip(FileDescriptor *const fp, const size_t size) + FUNC_ATTR_NONNULL_ALL +{ + size_t read_bytes = 0; + do { + ptrdiff_t new_read_bytes = file_read( + fp, skipbuf, (size_t)(size - read_bytes > sizeof(skipbuf) + ? sizeof(skipbuf) + : size - read_bytes)); + if (new_read_bytes < 0) { + return new_read_bytes; + } else if (new_read_bytes == 0) { + break; + } + read_bytes += (size_t)new_read_bytes; + } while (read_bytes < size && !fp->eof); + + return (ptrdiff_t)read_bytes; +} diff --git a/src/nvim/file.h b/src/nvim/file.h new file mode 100644 index 0000000000..0aa98e0def --- /dev/null +++ b/src/nvim/file.h @@ -0,0 +1,59 @@ +#ifndef NVIM_FILE_H +#define NVIM_FILE_H + +#include +#include +#include + +#include "nvim/func_attr.h" + +/// Structure used to read from/write to file +typedef struct { + int fd; ///< File descriptor. + bool eof; ///< True if end of file was encountered. +} FileDescriptor; + +/// file_open() flags +typedef enum { + FILE_READ_ONLY = O_RDONLY, ///< Open file read-only. + FILE_CREATE = O_CREAT, ///< Create file if it does not exist yet. + FILE_WRITE_ONLY = O_WRONLY, ///< Open file for writing only. +#ifdef O_NOFOLLOW + FILE_NOSYMLINK = O_NOFOLLOW, ///< Do not allow symbolic links. +#else + FILE_NOSYMLINK = 0, +#endif + FILE_CREATE_ONLY = O_CREAT|O_EXCL, ///< Only create the file, failing + ///< if it already exists. + FILE_TRUNCATE = O_TRUNC, ///< Truncate the file if it exists. +} FileOpenFlags; + +/// Check whether end of file was encountered +/// +/// @param[in] fp File to check. +/// +/// @return true if it was, false if it was not or read operation was never +/// performed. +static inline bool file_eof(const FileDescriptor *const fp) + FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ALL + FUNC_ATTR_ALWAYS_INLINE +{ + return fp->eof && rbuffer_size(fp->rv) == 0; +} + +/// Return the file descriptor associated with the FileDescriptor structure +/// +/// @param[in] fp File to check. +/// +/// @return File descriptor. +static inline int file_fd(const FileDescriptor *const fp) + FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ALL + FUNC_ATTR_ALWAYS_INLINE +{ + return fp->fd; +} + +#ifdef INCLUDE_GENERATED_DECLARATIONS +# include "file.h.generated.h" +#endif +#endif // NVIM_FILE_H diff --git a/src/nvim/os/fs.c b/src/nvim/os/fs.c index 08122828bd..1fd3987b97 100644 --- a/src/nvim/os/fs.c +++ b/src/nvim/os/fs.c @@ -1,14 +1,20 @@ // fs.c -- filesystem access #include - +#include #include +#include +#include #include +#include + +#include #include "nvim/os/os.h" #include "nvim/os/os_defs.h" #include "nvim/ascii.h" #include "nvim/memory.h" #include "nvim/message.h" +#include "nvim/assert.h" #include "nvim/misc1.h" #include "nvim/misc2.h" #include "nvim/path.h" @@ -18,6 +24,20 @@ # include "os/fs.c.generated.h" #endif +#define RUN_UV_FS_FUNC(ret, func, ...) \ + do { \ + bool did_try_to_free = false; \ +uv_call_start: {} \ + uv_fs_t req; \ + ret = func(&fs_loop, &req, __VA_ARGS__); \ + uv_fs_req_cleanup(&req); \ + if (ret == UV_ENOMEM && !did_try_to_free) { \ + try_to_free_memory(); \ + did_try_to_free = true; \ + goto uv_call_start; \ + } \ + } while (0) + // Many fs functions from libuv return that value on success. static const int kLibuvSuccess = 0; static uv_loop_t fs_loop; @@ -325,13 +345,123 @@ static bool is_executable_in_path(const char_u *name, char_u **abspath) int os_open(const char* path, int flags, int mode) FUNC_ATTR_NONNULL_ALL { - uv_fs_t open_req; - int r = uv_fs_open(&fs_loop, &open_req, path, flags, mode, NULL); - uv_fs_req_cleanup(&open_req); - // r is the same as open_req.result (except for OOM: then only r is set). + int r; + RUN_UV_FS_FUNC(r, uv_fs_open, path, flags, mode, NULL); return r; } +/// Close a file +/// +/// @return 0 or libuv error code on failure. +int os_close(const int fd) +{ + int r; + RUN_UV_FS_FUNC(r, uv_fs_close, fd, NULL); + return r; +} + +/// Read from a file +/// +/// Handles EINTR and ENOMEM, but not other errors. +/// +/// @param[in] fd File descriptor to read from. +/// @param[out] ret_eof Is set to true if EOF was encountered, otherwise set +/// to false. Initial value is ignored. +/// @param[out] ret_buf Buffer to write to. May be NULL if size is zero. +/// @param[in] size Amount of bytes to read. +/// +/// @return Number of bytes read or libuv error code (< 0). +ptrdiff_t os_read(const int fd, bool *ret_eof, char *const ret_buf, + const size_t size) + FUNC_ATTR_WARN_UNUSED_RESULT +{ + *ret_eof = false; + if (ret_buf == NULL) { + assert(size == 0); + return 0; + } + size_t read_bytes = 0; + bool did_try_to_free = false; + while (read_bytes != size) { + const ptrdiff_t cur_read_bytes = read(fd, ret_buf + read_bytes, + size - read_bytes); + if (cur_read_bytes > 0) { + read_bytes += (size_t) cur_read_bytes; + assert(read_bytes <= size); + } + if (cur_read_bytes < 0) { +#ifdef HAVE_UV_TRANSLATE_SYS_ERROR + const int error = uv_translate_sys_error(errno); +#else + const int error = -errno; + STATIC_ASSERT(-EINTR == UV_EINTR, "Need to translate error codes"); + STATIC_ASSERT(-EAGAIN == UV_EAGAIN, "Need to translate error codes"); + STATIC_ASSERT(-ENOMEM == UV_ENOMEM, "Need to translate error codes"); +#endif + errno = 0; + if (error == UV_EINTR || error == UV_EAGAIN) { + continue; + } else if (error == UV_ENOMEM && !did_try_to_free) { + try_to_free_memory(); + did_try_to_free = true; + continue; + } else { + return (ptrdiff_t) error; + } + } + if (cur_read_bytes == 0) { + *ret_eof = true; + break; + } + } + return (ptrdiff_t) read_bytes; +} + +/// Write to a file +/// +/// @param[in] fd File descriptor to write to. +/// @param[in] buf Data to write. May be NULL if size is zero. +/// @param[in] size Amount of bytes to write. +/// +/// @return Number of bytes written or libuv error code (< 0). +ptrdiff_t os_write(const int fd, const char *const buf, const size_t size) + FUNC_ATTR_WARN_UNUSED_RESULT +{ + if (buf == NULL) { + assert(size == 0); + return 0; + } + size_t written_bytes = 0; + while (written_bytes != size) { + const ptrdiff_t cur_written_bytes = write(fd, buf + written_bytes, + size - written_bytes); + if (cur_written_bytes > 0) { + written_bytes += (size_t) cur_written_bytes; + } + if (cur_written_bytes < 0) { +#ifdef HAVE_UV_TRANSLATE_SYS_ERROR + const int error = uv_translate_sys_error(errno); +#else + const int error = -errno; + STATIC_ASSERT(-EINTR == UV_EINTR, "Need to translate error codes"); + STATIC_ASSERT(-EAGAIN == UV_EAGAIN, "Need to translate error codes"); + // According to the man page open() may fail with ENOMEM, but write() + // can’t. +#endif + errno = 0; + if (error == UV_EINTR || error == UV_EAGAIN) { + continue; + } else { + return error; + } + } + if (cur_written_bytes == 0) { + return UV_UNKNOWN; + } + } + return (ptrdiff_t) written_bytes; +} + /// Flushes file modifications to disk. /// /// @param fd the file descriptor of the file to flush to disk. @@ -339,9 +469,8 @@ int os_open(const char* path, int flags, int mode) /// @return `0` on success, a libuv error code on failure. int os_fsync(int fd) { - uv_fs_t fsync_req; - int r = uv_fs_fsync(&fs_loop, &fsync_req, fd, NULL); - uv_fs_req_cleanup(&fsync_req); + int r; + RUN_UV_FS_FUNC(r, uv_fs_fsync, fd, NULL); return r; } @@ -379,16 +508,9 @@ int32_t os_getperm(const char_u *name) int os_setperm(const char_u *name, int perm) FUNC_ATTR_NONNULL_ALL { - uv_fs_t request; - int result = uv_fs_chmod(&fs_loop, &request, - (const char*)name, perm, NULL); - uv_fs_req_cleanup(&request); - - if (result == kLibuvSuccess) { - return OK; - } - - return FAIL; + int r; + RUN_UV_FS_FUNC(r, uv_fs_chmod, (const char *)name, perm, NULL); + return (r == kLibuvSuccess ? OK : FAIL); } /// Changes the ownership of the file referred to by the open file descriptor. @@ -397,13 +519,11 @@ int os_setperm(const char_u *name, int perm) /// /// @note If the `owner` or `group` is specified as `-1`, then that ID is not /// changed. -int os_fchown(int file_descriptor, uv_uid_t owner, uv_gid_t group) +int os_fchown(int fd, uv_uid_t owner, uv_gid_t group) { - uv_fs_t request; - int result = uv_fs_fchown(&fs_loop, &request, file_descriptor, - owner, group, NULL); - uv_fs_req_cleanup(&request); - return result; + int r; + RUN_UV_FS_FUNC(r, uv_fs_fchown, fd, owner, group, NULL); + return r; } /// Check if a file exists. @@ -422,9 +542,8 @@ bool os_file_exists(const char_u *name) bool os_file_is_readable(const char *name) FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT { - uv_fs_t req; - int r = uv_fs_access(&fs_loop, &req, name, R_OK, NULL); - uv_fs_req_cleanup(&req); + int r; + RUN_UV_FS_FUNC(r, uv_fs_access, name, R_OK, NULL); return (r == 0); } @@ -436,9 +555,8 @@ bool os_file_is_readable(const char *name) int os_file_is_writable(const char *name) FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT { - uv_fs_t req; - int r = uv_fs_access(&fs_loop, &req, name, W_OK, NULL); - uv_fs_req_cleanup(&req); + int r; + RUN_UV_FS_FUNC(r, uv_fs_access, name, W_OK, NULL); if (r == 0) { return os_isdir((char_u *)name) ? 2 : 1; } @@ -451,16 +569,10 @@ int os_file_is_writable(const char *name) int os_rename(const char_u *path, const char_u *new_path) FUNC_ATTR_NONNULL_ALL { - uv_fs_t request; - int result = uv_fs_rename(&fs_loop, &request, - (const char *)path, (const char *)new_path, NULL); - uv_fs_req_cleanup(&request); - - if (result == kLibuvSuccess) { - return OK; - } - - return FAIL; + int r; + RUN_UV_FS_FUNC(r, uv_fs_rename, (const char *)path, (const char *)new_path, + NULL); + return (r == kLibuvSuccess ? OK : FAIL); } /// Make a directory. @@ -469,10 +581,9 @@ int os_rename(const char_u *path, const char_u *new_path) int os_mkdir(const char *path, int32_t mode) FUNC_ATTR_NONNULL_ALL { - uv_fs_t request; - int result = uv_fs_mkdir(&fs_loop, &request, path, mode, NULL); - uv_fs_req_cleanup(&request); - return result; + int r; + RUN_UV_FS_FUNC(r, uv_fs_mkdir, path, mode, NULL); + return r; } /// Make a directory, with higher levels when needed @@ -554,10 +665,9 @@ int os_mkdtemp(const char *template, char *path) int os_rmdir(const char *path) FUNC_ATTR_NONNULL_ALL { - uv_fs_t request; - int result = uv_fs_rmdir(&fs_loop, &request, path, NULL); - uv_fs_req_cleanup(&request); - return result; + int r; + RUN_UV_FS_FUNC(r, uv_fs_rmdir, path, NULL); + return r; } /// Opens a directory. @@ -599,10 +709,9 @@ void os_closedir(Directory *dir) int os_remove(const char *path) FUNC_ATTR_NONNULL_ALL { - uv_fs_t request; - int result = uv_fs_unlink(&fs_loop, &request, path, NULL); - uv_fs_req_cleanup(&request); - return result; + int r; + RUN_UV_FS_FUNC(r, uv_fs_unlink, path, NULL); + return r; } /// Get the file information for a given path diff --git a/src/nvim/shada.c b/src/nvim/shada.c index 380d955f63..a8efaeb7cf 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -5,7 +5,6 @@ #include #include #include -#include #include #include @@ -36,6 +35,7 @@ #include "nvim/version.h" #include "nvim/path.h" #include "nvim/fileio.h" +#include "nvim/file.h" #include "nvim/strings.h" #include "nvim/quickfix.h" #include "nvim/eval/encode.h" @@ -409,7 +409,7 @@ typedef struct sd_read_def { ShaDaFileSkipper skip; ///< Function used to skip some bytes. void *cookie; ///< Data describing object read from. bool eof; ///< True if reader reached end of file. - char *error; ///< Error message in case of error. + const char *error; ///< Error message in case of error. uintmax_t fpos; ///< Current position (amount of bytes read since ///< reader structure initialization). May overflow. vimconv_T sd_conv; ///< Structure used for converting encodings of some @@ -433,7 +433,7 @@ typedef struct sd_write_def { ShaDaFileWriter write; ///< Writer function. ShaDaWriteCloser close; ///< Close function. void *cookie; ///< Data describing object written to. - char *error; ///< Error message in case of error. + const char *error; ///< Error message in case of error. vimconv_T sd_conv; ///< Structure used for converting encodings of some ///< items. } ShaDaWriteDef; @@ -666,38 +666,14 @@ static ptrdiff_t read_file(ShaDaReadDef *const sd_reader, void *const dest, const size_t size) FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT { - size_t read_bytes = 0; - bool did_try_to_free = false; - const int fd = (int)(intptr_t) sd_reader->cookie; - while (read_bytes != size) { - const ptrdiff_t cur_read_bytes = read(fd, ((char *) dest) + read_bytes, - size - read_bytes); - if (cur_read_bytes > 0) { - read_bytes += (size_t) cur_read_bytes; - sd_reader->fpos += (uintmax_t) cur_read_bytes; - assert(read_bytes <= size); - } - if (cur_read_bytes < 0) { - if (errno == EINTR || errno == EAGAIN) { - errno = 0; - continue; - } else if (errno == ENOMEM && !did_try_to_free) { - try_to_free_memory(); - did_try_to_free = true; - errno = 0; - continue; - } else { - sd_reader->error = strerror(errno); - errno = 0; - return -1; - } - } - if (cur_read_bytes == 0) { - sd_reader->eof = true; - break; - } + const ptrdiff_t ret = file_read(sd_reader->cookie, dest, size); + sd_reader->eof = file_eof(sd_reader->cookie); + if (ret < 0) { + sd_reader->error = os_strerror((int)ret); + return -1; } - return (ptrdiff_t) read_bytes; + sd_reader->fpos += (size_t) ret; + return ret; } /// Read one character @@ -720,50 +696,31 @@ static ptrdiff_t write_file(ShaDaWriteDef *const sd_writer, const size_t size) FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT { - size_t written_bytes = 0; - const int fd = (int)(intptr_t) sd_writer->cookie; - while (written_bytes != size) { - const ptrdiff_t cur_written_bytes = write(fd, (char *) dest + written_bytes, - size - written_bytes); - if (cur_written_bytes > 0) { - written_bytes += (size_t) cur_written_bytes; - } - if (cur_written_bytes < 0) { - if (errno == EINTR || errno == EAGAIN) { - errno = 0; - continue; - } else { - sd_writer->error = strerror(errno); - errno = 0; - return -1; - } - } - if (cur_written_bytes == 0) { - sd_writer->error = "Zero bytes written."; - return -1; - } + const ptrdiff_t ret = file_write(sd_writer->cookie, dest, size); + if (ret < 0) { + sd_writer->error = os_strerror((int)ret); + return -1; } - return (ptrdiff_t) written_bytes; + return ret; } /// Wrapper for closing file descriptors opened for reading static void close_sd_reader(ShaDaReadDef *const sd_reader) FUNC_ATTR_NONNULL_ALL { - close_file((int)(intptr_t) sd_reader->cookie); + close_file(sd_reader->cookie); } /// Wrapper for closing file descriptors opened for writing static void close_sd_writer(ShaDaWriteDef *const sd_writer) FUNC_ATTR_NONNULL_ALL { - const int fd = (int)(intptr_t) sd_writer->cookie; - if (os_fsync(fd) < 0) { + const int error = file_fsync(sd_writer->cookie); + if (error < 0) { emsgf(_(SERR "System error while synchronizing ShaDa file: %s"), - os_strerror(errno)); - errno = 0; + os_strerror(error)); } - close_file(fd); + close_file(sd_writer->cookie); } /// Wrapper for read that reads to IObuff and ignores bytes read @@ -779,19 +736,20 @@ static int sd_reader_skip_read(ShaDaReadDef *const sd_reader, const size_t offset) FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT { - size_t read_bytes = 0; - do { - ptrdiff_t new_read_bytes = sd_reader->read( - sd_reader, IObuff, (size_t) (offset - read_bytes > IOSIZE - ? IOSIZE - : offset - read_bytes)); - if (new_read_bytes == -1) { - return FAIL; + const ptrdiff_t skip_bytes = file_skip(sd_reader->cookie, offset); + if (skip_bytes < 0) { + sd_reader->error = os_strerror((int)skip_bytes); + return FAIL; + } else if (skip_bytes != (ptrdiff_t)offset) { + assert(skip_bytes < (ptrdiff_t)offset); + sd_reader->eof = file_eof(sd_reader->cookie); + if (!sd_reader->eof) { + sd_reader->error = _("too few bytes read"); } - read_bytes += (size_t) new_read_bytes; - } while (read_bytes < offset && !sd_reader->eof); - - return (read_bytes == offset ? OK : FAIL); + return FAIL; + } + sd_reader->fpos += (size_t)skip_bytes; + return OK; } /// Wrapper for read that can be used when lseek cannot be used @@ -824,37 +782,6 @@ static ShaDaReadResult sd_reader_skip(ShaDaReadDef *const sd_reader, return kSDReadStatusSuccess; } -/// Wrapper for opening file descriptors -/// -/// All arguments are passed to os_open(). -/// -/// @return file descriptor or libuv error on failure. -static int open_file(const char *const fname, const int flags, const int mode) - FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ALL -{ - bool did_try_to_free = false; - int fd; -open_file_start: - fd = os_open(fname, flags, mode); - - if (fd < 0) { - if (fd == UV_ENOENT) { - return fd; - } - if (fd == UV_ENOMEM && !did_try_to_free) { - try_to_free_memory(); - did_try_to_free = true; - goto open_file_start; - } - if (fd != UV_EEXIST) { - emsgf(_(SERR "System error while opening ShaDa file %s: %s"), - fname, os_strerror(fd)); - } - return fd; - } - return fd; -} - /// Open ShaDa file for reading /// /// @param[in] fname File name to open. @@ -865,11 +792,7 @@ static int open_shada_file_for_reading(const char *const fname, ShaDaReadDef *sd_reader) FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ALL { - const intptr_t fd = (intptr_t) open_file(fname, O_RDONLY, 0); - - if (fd < 0) { - return (int) fd; - } + int error; *sd_reader = (ShaDaReadDef) { .read = &read_file, @@ -878,8 +801,11 @@ static int open_shada_file_for_reading(const char *const fname, .error = NULL, .eof = false, .fpos = 0, - .cookie = (void *) fd, + .cookie = file_open_new(&error, fname, FILE_READ_ONLY, 0), }; + if (sd_reader->cookie == NULL) { + return error; + } convert_setup(&sd_reader->sd_conv, "utf-8", p_enc); @@ -887,18 +813,12 @@ static int open_shada_file_for_reading(const char *const fname, } /// Wrapper for closing file descriptors -static void close_file(int fd) +static void close_file(void *cookie) { -close_file_start: - if (close(fd) == -1) { - if (errno == EINTR) { - errno = 0; - goto close_file_start; - } else { - emsgf(_(SERR "System error while closing ShaDa file: %s"), - strerror(errno)); - errno = 0; - } + const int error = file_free(cookie); + if (error != 0) { + emsgf(_(SERR "System error while closing ShaDa file: %s"), + os_strerror(error)); } } @@ -978,7 +898,7 @@ static int shada_read_file(const char *const file, const int flags) } if (of_ret != 0) { - if (of_ret == UV_ENOENT && (flags & kShaDaMissingError)) { + if (of_ret != UV_ENOENT || (flags & kShaDaMissingError)) { emsgf(_(SERR "System error while opening ShaDa file %s for reading: %s"), fname, os_strerror(of_ret)); } @@ -2975,10 +2895,16 @@ int shada_write_file(const char *const file, bool nomerge) }; ShaDaReadDef sd_reader; - intptr_t fd; - if (!nomerge) { - if (open_shada_file_for_reading(fname, &sd_reader) != 0) { + int error; + if ((error = open_shada_file_for_reading(fname, &sd_reader)) != 0) { + if (error != UV_ENOENT) { + emsgf(_(SERR "System error while opening ShaDa file %s for reading " + "to merge before writing it: %s"), + fname, os_strerror(error)); + // Try writing the file even if opening it emerged any issues besides + // file not existing: maybe writing will succeed nevertheless. + } nomerge = true; goto shada_write_file_nomerge; } @@ -2996,15 +2922,12 @@ int shada_write_file(const char *const file, bool nomerge) // 2: Make sure that user can always read and write the result. // 3: If somebody happened to delete the file after it was opened for // reading use u=rw permissions. -shada_write_file_open: - fd = (intptr_t) open_file(tempname, O_CREAT|O_WRONLY|O_NOFOLLOW|O_EXCL, - perm); - if (fd < 0) { - if (fd == UV_EEXIST -#ifdef ELOOP - || fd == UV_ELOOP -#endif - ) { +shada_write_file_open: {} + sd_writer.cookie = file_open_new( + &error, tempname, FILE_CREATE_ONLY|FILE_NOSYMLINK|FILE_WRITE_ONLY, + perm); + if (sd_writer.cookie == NULL) { + if (error == UV_EEXIST || error == UV_ELOOP) { // File already exists, try another name char *const wp = tempname + strlen(tempname) - 1; if (*wp == 'z') { @@ -3019,6 +2942,9 @@ shada_write_file_open: (*wp)++; goto shada_write_file_open; } + } else { + emsgf(_(SERR "System error while opening temporary ShaDa file %s " + "for writing: %s"), tempname, os_strerror(error)); } } } @@ -3042,8 +2968,19 @@ shada_write_file_nomerge: {} } *tail = tail_save; } - fd = (intptr_t) open_file(fname, O_CREAT|O_WRONLY|O_TRUNC, - 0600); + int error; + sd_writer.cookie = file_open_new( + &error, fname, FILE_CREATE|FILE_WRITE_ONLY|FILE_TRUNCATE, 0600); + if (sd_writer.cookie == NULL) { + emsgf(_(SERR "System error while opening ShaDa file %s for writing: %s"), + fname, os_strerror(error)); + } + } + + if (sd_writer.cookie == NULL) { + xfree(fname); + xfree(tempname); + return FAIL; } if (p_verbose > 0) { @@ -3052,14 +2989,6 @@ shada_write_file_nomerge: {} verbose_leave(); } - if (fd < 0) { - xfree(fname); - xfree(tempname); - return FAIL; - } - - sd_writer.cookie = (void *) fd; - convert_setup(&sd_writer.sd_conv, p_enc, "utf-8"); const ShaDaWriteResult sw_ret = shada_write(&sd_writer, (nomerge @@ -3085,7 +3014,8 @@ shada_write_file_nomerge: {} || old_info.stat.st_gid != getgid()) { const uv_uid_t old_uid = (uv_uid_t) old_info.stat.st_uid; const uv_gid_t old_gid = (uv_gid_t) old_info.stat.st_gid; - const int fchown_ret = os_fchown((int) fd, old_uid, old_gid); + const int fchown_ret = os_fchown(file_fd(sd_writer.cookie), + old_uid, old_gid); sd_writer.close(&sd_writer); if (fchown_ret != 0) { EMSG3(_(RNERR "Failed setting uid and gid for file %s: %s"), From 516b7071cafd831ea563b73f6953232f5674cd0c Mon Sep 17 00:00:00 2001 From: ZyX Date: Sat, 4 Jun 2016 22:48:29 +0300 Subject: [PATCH 02/13] file: Add buffered reading and writing Still no busted tests. Not tested without HAVE_PREADV. --- config/CMakeLists.txt | 2 + config/config.h.in | 7 +++ src/nvim/file.c | 139 ++++++++++++++++++++++++++++++++++++++++-- src/nvim/file.h | 4 ++ src/nvim/os/fs.c | 73 ++++++++++++++++++++++ src/nvim/rbuffer.c | 2 +- src/nvim/shada.c | 40 ++++-------- 7 files changed, 234 insertions(+), 33 deletions(-) diff --git a/config/CMakeLists.txt b/config/CMakeLists.txt index e1e90c6a9d..cf84f8c6a4 100644 --- a/config/CMakeLists.txt +++ b/config/CMakeLists.txt @@ -27,6 +27,7 @@ if(NOT HAVE_SYS_WAIT_H AND UNIX) endif() check_include_files(sys/utsname.h HAVE_SYS_UTSNAME_H) check_include_files(utime.h HAVE_UTIME_H) +check_include_files(sys/uio.h HAVE_SYS_UIO_H) # Functions check_function_exists(fseeko HAVE_FSEEKO) @@ -34,6 +35,7 @@ check_function_exists(getpwent HAVE_GETPWENT) check_function_exists(getpwnam HAVE_GETPWNAM) check_function_exists(getpwuid HAVE_GETPWUID) check_function_exists(uv_translate_sys_error HAVE_UV_TRANSLATE_SYS_ERROR) +check_function_exists(readv HAVE_READV) if(Iconv_FOUND) set(HAVE_ICONV 1) diff --git a/config/config.h.in b/config/config.h.in index 7f16fd1928..4c35b3b1cb 100644 --- a/config/config.h.in +++ b/config/config.h.in @@ -49,6 +49,13 @@ #cmakedefine HAVE_WORKING_LIBINTL #cmakedefine UNIX #cmakedefine USE_FNAME_CASE +#cmakedefine HAVE_SYS_UIO_H +#ifdef HAVE_SYS_UIO_H +#cmakedefine HAVE_READV +# ifndef HAVE_READV +# undef HAVE_SYS_UIO_H +# endif +#endif #cmakedefine FEAT_TUI diff --git a/src/nvim/file.c b/src/nvim/file.c index bc230ecf00..262343fe29 100644 --- a/src/nvim/file.c +++ b/src/nvim/file.c @@ -5,26 +5,39 @@ /// replacement. #include +#include #include #include +#include "auto/config.h" + +#ifdef HAVE_SYS_UIO_H +# include +#endif + #include #include "nvim/file.h" #include "nvim/memory.h" #include "nvim/os/os.h" #include "nvim/globals.h" +#include "nvim/rbuffer.h" +#include "nvim/macros.h" #ifdef INCLUDE_GENERATED_DECLARATIONS # include "file.c.generated.h" #endif +#define RWBUFSIZE (IOSIZE - 1) + /// Open file /// /// @param[out] ret_fp Address where information needed for reading from or /// writing to a file is saved /// @param[in] fname File name to open. -/// @param[in] flags Flags, @see FileOpenFlags. +/// @param[in] flags Flags, @see FileOpenFlags. Currently reading from and +/// writing to the file at once is not supported, so either +/// FILE_WRITE_ONLY or FILE_READ_ONLY is required. /// @param[in] mode Permissions for the newly created file (ignored if flags /// does not have FILE_CREATE\*). /// @@ -41,8 +54,15 @@ int file_open(FileDescriptor *const ret_fp, const char *const fname, return fd; } + ret_fp->wr = (bool)(!!(flags & FILE_WRITE_ONLY)); ret_fp->fd = fd; ret_fp->eof = false; + ret_fp->rv = rbuffer_new(RWBUFSIZE); + ret_fp->_error = 0; + if (ret_fp->wr) { + ret_fp->rv->data = ret_fp; + ret_fp->rv->full_cb = (rbuffer_callback)&file_rb_write_full_cb; + } return 0; } @@ -68,7 +88,7 @@ FileDescriptor *file_open_new(int *const error, const char *const fname, return fp; } -/// Close file +/// Close file and free its buffer /// /// @param[in,out] fp File to close. /// @@ -77,6 +97,7 @@ int file_close(FileDescriptor *const fp) FUNC_ATTR_NONNULL_ALL { const int error = file_fsync(fp); const int error2 = os_close(fp->fd); + rbuffer_free(fp->rv); if (error2 != 0) { return error2; } @@ -103,9 +124,45 @@ int file_free(FileDescriptor *const fp) FUNC_ATTR_NONNULL_ALL int file_fsync(FileDescriptor *const fp) FUNC_ATTR_NONNULL_ALL { + if (!fp->wr) { + return 0; + } + file_rb_write_full_cb(fp->rv, fp); + if (fp->_error != 0) { + const int error = fp->_error; + fp->_error = 0; + return error; + } return os_fsync(fp->fd); } +/// Buffer used for writing +/// +/// Like IObuff, but allows file_\* callers not to care about spoiling it. +static char writebuf[RWBUFSIZE]; + +/// Function run when RBuffer is full when writing to a file +/// +/// Actually does writing to the file, may also be invoked directly. +/// +/// @param[in,out] rv RBuffer instance used. +/// @param[in,out] fp File to work with. +static void file_rb_write_full_cb(RBuffer *const rv, FileDescriptor *const fp) + FUNC_ATTR_NONNULL_ALL +{ + assert(fp->wr); + assert(rv->data == (void *)fp); + const size_t read_bytes = rbuffer_read(rv, writebuf, RWBUFSIZE); + const ptrdiff_t wres = os_write(fp->fd, writebuf, read_bytes); + if (wres != (ptrdiff_t)read_bytes) { + if (wres >= 0) { + fp->_error = UV_EIO; + } else { + fp->_error = (int)wres; + } + } +} + /// Read from file /// /// @param[in,out] fp File to work with. @@ -118,7 +175,69 @@ ptrdiff_t file_read(FileDescriptor *const fp, char *const ret_buf, const size_t size) FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT { - return os_read(fp->fd, &fp->eof, ret_buf, size); + assert(!fp->wr); + char *buf = ret_buf; + size_t read_remaining = size; + RBuffer *const rv = fp->rv; + while (read_remaining) { + const size_t rv_size = rbuffer_size(rv); + if (rv_size > 0) { + const size_t rsize = rbuffer_read(rv, buf, MIN(rv_size, read_remaining)); + buf += rsize; + read_remaining -= rsize; + } + if (fp->eof) { + break; + } + if (read_remaining) { + assert(rbuffer_size(rv) == 0); + rbuffer_reset(rv); + if (read_remaining >= RWBUFSIZE) { +#ifdef HAVE_READV + // If there is readv() syscall, then take an opportunity to populate + // both target buffer and RBuffer at once, … + size_t read_count; + struct iovec iov[] = { + { .iov_base = buf, .iov_len = read_remaining }, + { .iov_base = rbuffer_read_ptr(rv, &read_count), .iov_len = RWBUFSIZE + }, + }; + assert(read_count == RWBUFSIZE); + const ptrdiff_t r_ret = os_readv(fp->fd, &fp->eof, iov, + ARRAY_SIZE(iov)); + if (r_ret > 0) { + if (r_ret > (ptrdiff_t)read_remaining) { + rbuffer_produced(rv, (size_t)(r_ret - (ptrdiff_t)read_remaining)); + } + } else if (r_ret < 0) { + return r_ret; + } +#else + // …otherwise leave RBuffer empty and populate only target buffer, + // because filtering information through rbuffer will be more syscalls. + const ptrdiff_t r_ret = os_read(fp->fd, &fp->eof, buf, read_remaining); + if (r_ret >= 0) { + read_remaining -= (size_t)r_ret; + return (ptrdiff_t)(size - read_remaining); + } else if (r_ret < 0) { + return r_ret; + } +#endif + } else { + size_t write_count; + const ptrdiff_t r_ret = os_read(fp->fd, &fp->eof, + rbuffer_write_ptr(rv, &write_count), + RWBUFSIZE); + assert(write_count == RWBUFSIZE); + if (r_ret > 0) { + rbuffer_produced(rv, (size_t)r_ret); + } else if (r_ret < 0) { + return r_ret; + } + } + } + } + return (ptrdiff_t)(size - read_remaining); } /// Write to a file @@ -132,12 +251,21 @@ ptrdiff_t file_write(FileDescriptor *const fp, const char *const buf, const size_t size) FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ARG(1) { - return os_write(fp->fd, buf, size); + assert(fp->wr); + const size_t written = rbuffer_write(fp->rv, buf, size); + if (fp->_error != 0) { + const int error = fp->_error; + fp->_error = 0; + return error; + } else if (written != size) { + return UV_EIO; + } + return (ptrdiff_t)written; } /// Buffer used for skipping. Its contents is undefined and should never be /// used. -static char skipbuf[IOSIZE]; +static char skipbuf[RWBUFSIZE]; /// Skip some bytes /// @@ -146,6 +274,7 @@ static char skipbuf[IOSIZE]; ptrdiff_t file_skip(FileDescriptor *const fp, const size_t size) FUNC_ATTR_NONNULL_ALL { + assert(!fp->wr); size_t read_bytes = 0; do { ptrdiff_t new_read_bytes = file_read( diff --git a/src/nvim/file.h b/src/nvim/file.h index 0aa98e0def..c6def673c2 100644 --- a/src/nvim/file.h +++ b/src/nvim/file.h @@ -6,10 +6,14 @@ #include #include "nvim/func_attr.h" +#include "nvim/rbuffer.h" /// Structure used to read from/write to file typedef struct { int fd; ///< File descriptor. + int _error; ///< Error code for use with RBuffer callbacks or zero. + RBuffer *rv; ///< Read or write buffer. + bool wr; ///< True if file is in write mode. bool eof; ///< True if end of file was encountered. } FileDescriptor; diff --git a/src/nvim/os/fs.c b/src/nvim/os/fs.c index 1fd3987b97..19d4cbc440 100644 --- a/src/nvim/os/fs.c +++ b/src/nvim/os/fs.c @@ -7,6 +7,12 @@ #include #include +#include "auto/config.h" + +#ifdef HAVE_SYS_UIO_H +# include +#endif + #include #include "nvim/os/os.h" @@ -417,6 +423,73 @@ ptrdiff_t os_read(const int fd, bool *ret_eof, char *const ret_buf, return (ptrdiff_t) read_bytes; } +#ifdef HAVE_READV +/// Read from a file to a multiple buffers at once +/// +/// Wrapper for readv(). +/// +/// @param[in] fd File descriptor to read from. +/// @param[out] ret_eof Is set to true if EOF was encountered, otherwise set +/// to false. Initial value is ignored. +/// @param[out] iov Description of buffers to write to. Note: this description +/// may change, it is incorrect to use data it points to after +/// os_readv(). +/// @param[in] iov_size Number of buffers in iov. +ptrdiff_t os_readv(int fd, bool *ret_eof, struct iovec *iov, size_t iov_size) + FUNC_ATTR_NONNULL_ALL +{ + *ret_eof = false; + size_t read_bytes = 0; + bool did_try_to_free = false; + size_t toread = 0; + for (size_t i = 0; i < iov_size; i++) { + // Overflow, trying to read too much data + assert(toread <= SIZE_MAX - iov[i].iov_len); + toread += iov[i].iov_len; + } + while (read_bytes < toread && iov_size && !*ret_eof) { + ptrdiff_t cur_read_bytes = readv(fd, iov, (int)iov_size); + if (toread && cur_read_bytes == 0) { + *ret_eof = true; + } + if (cur_read_bytes > 0) { + read_bytes += (size_t)cur_read_bytes; + while (iov_size && cur_read_bytes) { + if (cur_read_bytes < (ptrdiff_t) iov->iov_len) { + iov->iov_len -= (size_t)cur_read_bytes; + iov->iov_base = (char *)iov->iov_base + cur_read_bytes; + cur_read_bytes = 0; + } else { + cur_read_bytes -= (ptrdiff_t)iov->iov_len; + iov_size--; + iov++; + } + } + } else if (cur_read_bytes < 0) { +#ifdef HAVE_UV_TRANSLATE_SYS_ERROR + const int error = uv_translate_sys_error(errno); +#else + const int error = -errno; + STATIC_ASSERT(-EINTR == UV_EINTR, "Need to translate error codes"); + STATIC_ASSERT(-EAGAIN == UV_EAGAIN, "Need to translate error codes"); + STATIC_ASSERT(-ENOMEM == UV_ENOMEM, "Need to translate error codes"); +#endif + errno = 0; + if (error == UV_EINTR || error == UV_EAGAIN) { + continue; + } else if (error == UV_ENOMEM && !did_try_to_free) { + try_to_free_memory(); + did_try_to_free = true; + continue; + } else { + return (ptrdiff_t)error; + } + } + } + return (ptrdiff_t)read_bytes; +} +#endif // HAVE_READV + /// Write to a file /// /// @param[in] fd File descriptor to write to. diff --git a/src/nvim/rbuffer.c b/src/nvim/rbuffer.c index b3805a3a28..d4cc8c0d5d 100644 --- a/src/nvim/rbuffer.c +++ b/src/nvim/rbuffer.c @@ -153,7 +153,7 @@ void rbuffer_consumed(RBuffer *buf, size_t count) // Higher level functions for copying from/to RBuffer instances and data // pointers -size_t rbuffer_write(RBuffer *buf, char *src, size_t src_size) +size_t rbuffer_write(RBuffer *buf, const char *src, size_t src_size) FUNC_ATTR_NONNULL_ALL { size_t size = src_size; diff --git a/src/nvim/shada.c b/src/nvim/shada.c index a8efaeb7cf..130d8bbfae 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -715,11 +715,6 @@ static void close_sd_reader(ShaDaReadDef *const sd_reader) static void close_sd_writer(ShaDaWriteDef *const sd_writer) FUNC_ATTR_NONNULL_ALL { - const int error = file_fsync(sd_writer->cookie); - if (error < 0) { - emsgf(_(SERR "System error while synchronizing ShaDa file: %s"), - os_strerror(error)); - } close_file(sd_writer->cookie); } @@ -2995,15 +2990,11 @@ shada_write_file_nomerge: {} ? NULL : &sd_reader)); assert(sw_ret != kSDWriteIgnError); -#ifndef UNIX - sd_writer.close(&sd_writer); -#endif if (!nomerge) { sd_reader.close(&sd_reader); bool did_remove = false; if (sw_ret == kSDWriteSuccessfull) { #ifdef UNIX - bool closed = false; // For Unix we check the owner of the file. It's not very nice to // overwrite a user’s viminfo file after a "su root", with a // viminfo file that the user can't read. @@ -3016,13 +3007,11 @@ shada_write_file_nomerge: {} const uv_gid_t old_gid = (uv_gid_t) old_info.stat.st_gid; const int fchown_ret = os_fchown(file_fd(sd_writer.cookie), old_uid, old_gid); - sd_writer.close(&sd_writer); if (fchown_ret != 0) { EMSG3(_(RNERR "Failed setting uid and gid for file %s: %s"), tempname, os_strerror(fchown_ret)); goto shada_write_file_did_not_remove; } - closed = true; } } else if (!(old_info.stat.st_uid == getuid() ? (old_info.stat.st_mode & 0200) @@ -3030,13 +3019,9 @@ shada_write_file_nomerge: {} ? (old_info.stat.st_mode & 0020) : (old_info.stat.st_mode & 0002)))) { EMSG2(_("E137: ShaDa file is not writable: %s"), fname); - sd_writer.close(&sd_writer); goto shada_write_file_did_not_remove; } } - if (!closed) { - sd_writer.close(&sd_writer); - } #endif if (vim_rename(tempname, fname) == -1) { EMSG3(_(RNERR "Can't rename ShaDa file from %s to %s!"), @@ -3063,6 +3048,7 @@ shada_write_file_did_not_remove: } xfree(tempname); } + sd_writer.close(&sd_writer); xfree(fname); return OK; @@ -3192,20 +3178,20 @@ static ShaDaReadResult fread_len(ShaDaReadDef *const sd_reader, FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT { const ptrdiff_t read_bytes = sd_reader->read(sd_reader, buffer, length); - (void) read_bytes; - if (sd_reader->error != NULL) { - emsgf(_(SERR "System error while reading ShaDa file: %s"), - sd_reader->error); - return kSDReadStatusReadError; - } else if (sd_reader->eof) { - emsgf(_(RCERR "Error while reading ShaDa file: " - "last entry specified that it occupies %" PRIu64 " bytes, " - "but file ended earlier"), - (uint64_t) length); - return kSDReadStatusNotShaDa; + if (read_bytes != (ptrdiff_t)length) { + if (sd_reader->error != NULL) { + emsgf(_(SERR "System error while reading ShaDa file: %s"), + sd_reader->error); + return kSDReadStatusReadError; + } else { + emsgf(_(RCERR "Error while reading ShaDa file: " + "last entry specified that it occupies %" PRIu64 " bytes, " + "but file ended earlier"), + (uint64_t) length); + return kSDReadStatusNotShaDa; + } } - assert(read_bytes >= 0 && (size_t) read_bytes == length); return kSDReadStatusSuccess; } From 2ac5f1138b5c4c5540479e26ca9b9755cf7b784d Mon Sep 17 00:00:00 2001 From: ZyX Date: Sun, 5 Jun 2016 13:40:37 +0300 Subject: [PATCH 03/13] unittests: Add os_close, os_read and os_readv tests --- test/unit/os/fs_spec.lua | 218 +++++++++++++++++++++++++++++++++++---- 1 file changed, 200 insertions(+), 18 deletions(-) diff --git a/test/unit/os/fs_spec.lua b/test/unit/os/fs_spec.lua index 857a5001f1..54f5a99902 100644 --- a/test/unit/os/fs_spec.lua +++ b/test/unit/os/fs_spec.lua @@ -17,6 +17,10 @@ local NULL = helpers.NULL local NODE_NORMAL = 0 local NODE_WRITABLE = 1 +local function ok(expr) + assert.is_true(expr) +end + cimport('unistd.h') cimport('./src/nvim/os/shell.h') cimport('./src/nvim/option_defs.h') @@ -150,11 +154,11 @@ describe('fs function', function() local function os_can_exe(name) local buf = ffi.new('char *[1]') buf[0] = NULL - local ok = fs.os_can_exe(to_cstr(name), buf, true) + local ce_ret = fs.os_can_exe(to_cstr(name), buf, true) -- When os_can_exe returns true, it must set the path. -- When it returns false, the path must be NULL. - if ok then + if ce_ret then neq(NULL, buf[0]) return internalize(buf[0]) else @@ -368,6 +372,44 @@ describe('fs function', function() local function os_open(path, flags, mode) return fs.os_open((to_cstr(path)), flags, mode) end + local function os_close(fd) + return fs.os_close(fd) + end + local function os_read(fd, size) + local buf = nil + if size == nil then + size = 0 + else + buf = ffi.new('char[?]', size, ('\0'):rep(size)) + end + local eof = ffi.new('bool[?]', 1, {true}) + local ret2 = fs.os_read(fd, eof, buf, size) + local ret1 = eof[0] + local ret3 = '' + if buf ~= nil then + ret3 = ffi.string(buf, size) + end + return ret1, ret2, ret3 + end + local function os_readv(fd, sizes) + local bufs = {} + for i, size in ipairs(sizes) do + bufs[i] = { + iov_base=ffi.new('char[?]', size, ('\0'):rep(size)), + iov_len=size, + } + end + local iov = ffi.new('struct iovec[?]', #sizes, bufs) + local eof = ffi.new('bool[?]', 1, {true}) + local ret2 = fs.os_readv(fd, eof, iov, #sizes) + local ret1 = eof[0] + local ret3 = {} + for i = 1,#sizes do + -- Warning: iov may not be used. + ret3[i] = ffi.string(bufs[i].iov_base, bufs[i].iov_len) + end + return ret1, ret2, ret3 + end describe('os_file_exists', function() it('returns false when given a non-existing file', function() @@ -432,30 +474,34 @@ describe('fs function', function() end) describe('os_open', function() + local new_file = 'test_new_file' + local existing_file = 'unit-test-directory/test_existing.file' + before_each(function() - io.open('unit-test-directory/test_existing.file', 'w').close() + (io.open(existing_file, 'w')):close() end) after_each(function() - os.remove('unit-test-directory/test_existing.file') - os.remove('test_new_file') + os.remove(existing_file) + os.remove(new_file) end) - local new_file = 'test_new_file' - local existing_file = 'unit-test-directory/test_existing.file' - it('returns UV_ENOENT for O_RDWR on a non-existing file', function() eq(ffi.C.UV_ENOENT, (os_open('non-existing-file', ffi.C.kO_RDWR, 0))) end) - it('returns non-negative for O_CREAT on a non-existing file', function() + it('returns non-negative for O_CREAT on a non-existing file which then can be closed', function() assert_file_does_not_exist(new_file) - assert.is_true(0 <= (os_open(new_file, ffi.C.kO_CREAT, 0))) + local fd = os_open(new_file, ffi.C.kO_CREAT, 0) + assert.is_true(0 <= fd) + eq(0, os_close(fd)) end) - it('returns non-negative for O_CREAT on a existing file', function() + it('returns non-negative for O_CREAT on a existing file which then can be closed', function() assert_file_exists(existing_file) - assert.is_true(0 <= (os_open(existing_file, ffi.C.kO_CREAT, 0))) + local fd = os_open(existing_file, ffi.C.kO_CREAT, 0) + assert.is_true(0 <= fd) + eq(0, os_close(fd)) end) it('returns UV_EEXIST for O_CREAT|O_EXCL on a existing file', function() @@ -463,24 +509,160 @@ describe('fs function', function() eq(ffi.C.kUV_EEXIST, (os_open(existing_file, (bit.bor(ffi.C.kO_CREAT, ffi.C.kO_EXCL)), 0))) end) - it('sets `rwx` permissions for O_CREAT 700', function() + it('sets `rwx` permissions for O_CREAT 700 which then can be closed', function() assert_file_does_not_exist(new_file) --create the file - os_open(new_file, ffi.C.kO_CREAT, tonumber("700", 8)) + local fd = os_open(new_file, ffi.C.kO_CREAT, tonumber("700", 8)) --verify permissions eq('rwx------', lfs.attributes(new_file)['permissions']) + eq(0, os_close(fd)) end) - it('sets `rw` permissions for O_CREAT 600', function() + it('sets `rw` permissions for O_CREAT 600 which then can be closed', function() assert_file_does_not_exist(new_file) --create the file - os_open(new_file, ffi.C.kO_CREAT, tonumber("600", 8)) + local fd = os_open(new_file, ffi.C.kO_CREAT, tonumber("600", 8)) --verify permissions eq('rw-------', lfs.attributes(new_file)['permissions']) + eq(0, os_close(fd)) end) - it('returns a non-negative file descriptor for an existing file', function() - assert.is_true(0 <= (os_open(existing_file, ffi.C.kO_RDWR, 0))) + it('returns a non-negative file descriptor for an existing file which then can be closed', function() + local fd = os_open(existing_file, ffi.C.kO_RDWR, 0) + assert.is_true(0 <= fd) + eq(0, os_close(fd)) + end) + end) + + describe('os_close', function() + it('returns EBADF for negative file descriptors', function() + eq(ffi.C.UV_EBADF, os_close(-1)) + eq(ffi.C.UV_EBADF, os_close(-1000)) + end) + end) + + describe('os_read', function() + local file = 'test-unit-os-fs_spec-os_read.dat' + + local s = '' + for i = 0, 255 do + s = s .. (i == 0 and '\0' or ('%c'):format(i)) + end + local fcontents = s:rep(16) + + before_each(function() + local f = io.open(file, 'w') + f:write(fcontents) + f:close() + end) + + after_each(function() + os.remove(file) + end) + + it('can read zero bytes from a file', function() + local fd = os_open(file, ffi.C.kO_RDONLY, 0) + ok(fd >= 0) + eq({false, 0, ''}, {os_read(fd, nil)}) + eq({false, 0, ''}, {os_read(fd, 0)}) + eq(0, os_close(fd)) + end) + + it('can read from a file multiple times', function() + local fd = os_open(file, ffi.C.kO_RDONLY, 0) + ok(fd >= 0) + eq({false, 2, '\000\001'}, {os_read(fd, 2)}) + eq({false, 2, '\002\003'}, {os_read(fd, 2)}) + eq(0, os_close(fd)) + end) + + it('can read the whole file at once and then report eof', function() + local fd = os_open(file, ffi.C.kO_RDONLY, 0) + ok(fd >= 0) + eq({false, #fcontents, fcontents}, {os_read(fd, #fcontents)}) + eq({true, 0, ('\0'):rep(#fcontents)}, {os_read(fd, #fcontents)}) + eq(0, os_close(fd)) + end) + + it('can read the whole file in two calls, one partially', function() + local fd = os_open(file, ffi.C.kO_RDONLY, 0) + ok(fd >= 0) + eq({false, #fcontents * 3/4, fcontents:sub(1, #fcontents * 3/4)}, + {os_read(fd, #fcontents * 3/4)}) + eq({true, + (#fcontents * 1/4), + fcontents:sub(#fcontents * 3/4 + 1) .. ('\0'):rep(#fcontents * 2/4)}, + {os_read(fd, #fcontents * 3/4)}) + eq(0, os_close(fd)) + end) + end) + + describe('os_readv', function() + -- Function may be absent + if not pcall(function() return fs.os_readv end) then + return + end + local file = 'test-unit-os-fs_spec-os_readv.dat' + + local s = '' + for i = 0, 255 do + s = s .. (i == 0 and '\0' or ('%c'):format(i)) + end + local fcontents = s:rep(16) + + before_each(function() + local f = io.open(file, 'w') + f:write(fcontents) + f:close() + end) + + after_each(function() + os.remove(file) + end) + + it('can read zero bytes from a file', function() + local fd = os_open(file, ffi.C.kO_RDONLY, 0) + ok(fd >= 0) + eq({false, 0, {}}, {os_readv(fd, {})}) + eq({false, 0, {'', '', ''}}, {os_readv(fd, {0, 0, 0})}) + eq(0, os_close(fd)) + end) + + it('can read from a file multiple times to a differently-sized buffers', function() + local fd = os_open(file, ffi.C.kO_RDONLY, 0) + ok(fd >= 0) + eq({false, 2, {'\000\001'}}, {os_readv(fd, {2})}) + eq({false, 5, {'\002\003', '\004\005\006'}}, {os_readv(fd, {2, 3})}) + eq(0, os_close(fd)) + end) + + it('can read the whole file at once and then report eof', function() + local fd = os_open(file, ffi.C.kO_RDONLY, 0) + ok(fd >= 0) + eq({false, + #fcontents, + {fcontents:sub(1, #fcontents * 1/4), + fcontents:sub(#fcontents * 1/4 + 1, #fcontents * 3/4), + fcontents:sub(#fcontents * 3/4 + 1, #fcontents * 15/16), + fcontents:sub(#fcontents * 15/16 + 1, #fcontents)}}, + {os_readv(fd, {#fcontents * 1/4, + #fcontents * 2/4, + #fcontents * 3/16, + #fcontents * 1/16})}) + eq({true, 0, {'\0'}}, {os_readv(fd, {1})}) + eq(0, os_close(fd)) + end) + + it('can read the whole file in two calls, one partially', function() + local fd = os_open(file, ffi.C.kO_RDONLY, 0) + ok(fd >= 0) + eq({false, #fcontents * 3/4, {fcontents:sub(1, #fcontents * 3/4)}}, + {os_readv(fd, {#fcontents * 3/4})}) + eq({true, + (#fcontents * 1/4), + {fcontents:sub(#fcontents * 3/4 + 1) .. ('\0'):rep(#fcontents * 2/4)}}, + {os_readv(fd, {#fcontents * 3/4})}) + eq(0, os_close(fd)) end) end) From a8f3849bc04cee5d7166138a28cb1e7bbf300803 Mon Sep 17 00:00:00 2001 From: ZyX Date: Tue, 21 Jun 2016 22:18:03 +0300 Subject: [PATCH 04/13] file: Use own constants, do not rely on fcntl.h One of the reasons is that O_RDONLY is zero, which makes checking whether file is opened read- or write-only harder. It is not guaranteed that on other system O_WRONLY will not be zero (e.g. because file can only be opened in read-write mode). --- src/nvim/file.c | 27 +++++++++++++++++++++++++-- src/nvim/file.h | 24 ++++++++++++------------ src/nvim/shada.c | 13 ++++++------- 3 files changed, 43 insertions(+), 21 deletions(-) diff --git a/src/nvim/file.c b/src/nvim/file.c index 262343fe29..35c659f7fd 100644 --- a/src/nvim/file.c +++ b/src/nvim/file.c @@ -8,6 +8,7 @@ #include #include #include +#include #include "auto/config.h" @@ -46,15 +47,37 @@ int file_open(FileDescriptor *const ret_fp, const char *const fname, const int flags, const int mode) FUNC_ATTR_NONNULL_ALL FUNC_ATTR_WARN_UNUSED_RESULT { + int os_open_flags = 0; int fd; + TriState wr = kNone; +#define FLAG(flags, flag, fcntl_flags, wrval, cond) \ + do { \ + if (flags & flag) { \ + os_open_flags |= fcntl_flags; \ + assert(cond); \ + if (wrval != kNone) { \ + wr = wrval; \ + } \ + } \ + } while (0) + FLAG(flags, kFileWriteOnly, O_WRONLY, kTrue, true); + FLAG(flags, kFileCreateOnly, O_CREAT|O_EXCL|O_WRONLY, kTrue, true); + FLAG(flags, kFileCreate, O_CREAT|O_WRONLY, kTrue, !(flags & kFileCreateOnly)); + FLAG(flags, kFileTruncate, O_TRUNC|O_WRONLY, kTrue, + !(flags & kFileCreateOnly)); + FLAG(flags, kFileReadOnly, O_RDONLY, kFalse, wr != kTrue); +#ifdef O_NOFOLLOW + FLAG(flags, kFileNoSymlink, O_NOFOLLOW, kNone, true); +#endif +#undef FLAG - fd = os_open(fname, flags, mode); + fd = os_open(fname, os_open_flags, mode); if (fd < 0) { return fd; } - ret_fp->wr = (bool)(!!(flags & FILE_WRITE_ONLY)); + ret_fp->wr = (wr == kTrue); ret_fp->fd = fd; ret_fp->eof = false; ret_fp->rv = rbuffer_new(RWBUFSIZE); diff --git a/src/nvim/file.h b/src/nvim/file.h index c6def673c2..5ee572750d 100644 --- a/src/nvim/file.h +++ b/src/nvim/file.h @@ -3,7 +3,6 @@ #include #include -#include #include "nvim/func_attr.h" #include "nvim/rbuffer.h" @@ -19,17 +18,18 @@ typedef struct { /// file_open() flags typedef enum { - FILE_READ_ONLY = O_RDONLY, ///< Open file read-only. - FILE_CREATE = O_CREAT, ///< Create file if it does not exist yet. - FILE_WRITE_ONLY = O_WRONLY, ///< Open file for writing only. -#ifdef O_NOFOLLOW - FILE_NOSYMLINK = O_NOFOLLOW, ///< Do not allow symbolic links. -#else - FILE_NOSYMLINK = 0, -#endif - FILE_CREATE_ONLY = O_CREAT|O_EXCL, ///< Only create the file, failing - ///< if it already exists. - FILE_TRUNCATE = O_TRUNC, ///< Truncate the file if it exists. + kFileReadOnly = 1, ///< Open file read-only. Default. + kFileCreate = 2, ///< Create file if it does not exist yet. + ///< Implies kFileWriteOnly. + kFileWriteOnly = 4, ///< Open file for writing only. + ///< Cannot be used with kFileReadOnly. + kFileNoSymlink = 8, ///< Do not allow symbolic links. + kFileCreateOnly = 16, ///< Only create the file, failing if it already + ///< exists. Implies kFileWriteOnly. Cannot be used + ///< with kFileCreate. + kFileTruncate = 32, ///< Truncate the file if it exists. + ///< Implies kFileWriteOnly. Cannot be used with + ///< kFileCreateOnly. } FileOpenFlags; /// Check whether end of file was encountered diff --git a/src/nvim/shada.c b/src/nvim/shada.c index 130d8bbfae..b4695603b2 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -672,7 +672,7 @@ static ptrdiff_t read_file(ShaDaReadDef *const sd_reader, void *const dest, sd_reader->error = os_strerror((int)ret); return -1; } - sd_reader->fpos += (size_t) ret; + sd_reader->fpos += (size_t)ret; return ret; } @@ -796,7 +796,7 @@ static int open_shada_file_for_reading(const char *const fname, .error = NULL, .eof = false, .fpos = 0, - .cookie = file_open_new(&error, fname, FILE_READ_ONLY, 0), + .cookie = file_open_new(&error, fname, kFileReadOnly, 0), }; if (sd_reader->cookie == NULL) { return error; @@ -1336,7 +1336,7 @@ static void shada_read(ShaDaReadDef *const sd_reader, const int flags) } } if (!op_register_set(cur_entry.data.reg.name, (yankreg_T) { - .y_array = (char_u **) cur_entry.data.reg.contents, + .y_array = (char_u **)cur_entry.data.reg.contents, .y_size = cur_entry.data.reg.contents_size, .y_type = cur_entry.data.reg.type, .y_width = (colnr_T) cur_entry.data.reg.width, @@ -2919,8 +2919,7 @@ int shada_write_file(const char *const file, bool nomerge) // reading use u=rw permissions. shada_write_file_open: {} sd_writer.cookie = file_open_new( - &error, tempname, FILE_CREATE_ONLY|FILE_NOSYMLINK|FILE_WRITE_ONLY, - perm); + &error, tempname, kFileCreateOnly|kFileNoSymlink, perm); if (sd_writer.cookie == NULL) { if (error == UV_EEXIST || error == UV_ELOOP) { // File already exists, try another name @@ -2964,8 +2963,8 @@ shada_write_file_nomerge: {} *tail = tail_save; } int error; - sd_writer.cookie = file_open_new( - &error, fname, FILE_CREATE|FILE_WRITE_ONLY|FILE_TRUNCATE, 0600); + sd_writer.cookie = file_open_new(&error, fname, kFileCreate|kFileTruncate, + 0600); if (sd_writer.cookie == NULL) { emsgf(_(SERR "System error while opening ShaDa file %s for writing: %s"), fname, os_strerror(error)); From 4b9d2caec21f2150e4b253d5201809d77a1e0912 Mon Sep 17 00:00:00 2001 From: ZyX Date: Tue, 21 Jun 2016 22:40:09 +0300 Subject: [PATCH 05/13] shada: Do not forget to close ShaDa reader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously there was file descriptor leak, not detected by sanitizers. Now it is file descriptor leak with a small memory leak which is detected by ASAN what fails one of the tests (actually, “ShaDa support code leaves .tmp.z in-place when there is error in original ShaDa and it has .tmp.a … .tmp.x”, but error is reported at the next test because leaks are not detected until Neovim exit and Neovim exit happens when clear()/reset() is called which happens in before_each only). --- src/nvim/file.c | 2 +- src/nvim/shada.c | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/nvim/file.c b/src/nvim/file.c index 35c659f7fd..2fb0e1cd87 100644 --- a/src/nvim/file.c +++ b/src/nvim/file.c @@ -310,7 +310,7 @@ ptrdiff_t file_skip(FileDescriptor *const fp, const size_t size) break; } read_bytes += (size_t)new_read_bytes; - } while (read_bytes < size && !fp->eof); + } while (read_bytes < size && !file_eof(fp)); return (ptrdiff_t)read_bytes; } diff --git a/src/nvim/shada.c b/src/nvim/shada.c index b4695603b2..a833b958b9 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -2883,12 +2883,12 @@ int shada_write_file(const char *const file, bool nomerge) char *const fname = shada_filename(file); char *tempname = NULL; - ShaDaWriteDef sd_writer = (ShaDaWriteDef) { + ShaDaWriteDef sd_writer = { .write = &write_file, .close = &close_sd_writer, .error = NULL, }; - ShaDaReadDef sd_reader; + ShaDaReadDef sd_reader = { .close = NULL }; if (!nomerge) { int error; @@ -2931,6 +2931,8 @@ shada_write_file_open: {} fname); xfree(fname); xfree(tempname); + assert(sd_reader.close != NULL); + sd_reader.close(&sd_reader); return FAIL; } else { (*wp)++; @@ -2974,6 +2976,9 @@ shada_write_file_nomerge: {} if (sd_writer.cookie == NULL) { xfree(fname); xfree(tempname); + if (sd_reader.close != NULL) { + sd_reader.close(&sd_reader); + } return FAIL; } From 3e7c8e714915672a032eef2950b25264a3c91c58 Mon Sep 17 00:00:00 2001 From: ZyX Date: Tue, 21 Jun 2016 22:51:40 +0300 Subject: [PATCH 06/13] unittests: Add os_write test New os/fs.c functions are now all tested. --- test/unit/os/fs_spec.lua | 42 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/test/unit/os/fs_spec.lua b/test/unit/os/fs_spec.lua index 54f5a99902..d7e98b43ce 100644 --- a/test/unit/os/fs_spec.lua +++ b/test/unit/os/fs_spec.lua @@ -410,6 +410,9 @@ describe('fs function', function() end return ret1, ret2, ret3 end + local function os_write(fd, data) + return fs.os_write(fd, data, data and #data or 0) + end describe('os_file_exists', function() it('returns false when given a non-existing file', function() @@ -666,6 +669,45 @@ describe('fs function', function() end) end) + describe('os_write', function() + -- Function may be absent + local file = 'test-unit-os-fs_spec-os_write.dat' + + local s = '' + for i = 0, 255 do + s = s .. (i == 0 and '\0' or ('%c'):format(i)) + end + local fcontents = s:rep(16) + + before_each(function() + local f = io.open(file, 'w') + f:write(fcontents) + f:close() + end) + + after_each(function() + os.remove(file) + end) + + it('can write zero bytes to a file', function() + local fd = os_open(file, ffi.C.kO_WRONLY, 0) + ok(fd >= 0) + eq(0, os_write(fd, '')) + eq(0, os_write(fd, nil)) + eq(fcontents, io.open(file, 'r'):read('*a')) + eq(0, os_close(fd)) + end) + + it('can write some data to a file', function() + local fd = os_open(file, ffi.C.kO_WRONLY, 0) + ok(fd >= 0) + eq(3, os_write(fd, 'abc')) + eq(4, os_write(fd, ' def')) + eq('abc def' .. fcontents:sub(8), io.open(file, 'r'):read('*a')) + eq(0, os_close(fd)) + end) + end) + describe('os_nodetype', function() before_each(function() os.remove('non-existing-file') From fec7983ecd7ac58f4f8ad9f8b4dc8f6937b87099 Mon Sep 17 00:00:00 2001 From: ZyX Date: Wed, 22 Jun 2016 01:01:21 +0300 Subject: [PATCH 07/13] unittests: Add tests for file.c Also fixes some errors found. --- src/nvim/file.c | 36 ++-- src/nvim/file.h | 17 +- test/unit/file_spec.lua | 361 +++++++++++++++++++++++++++++++++++++++ test/unit/helpers.lua | 5 +- test/unit/os/fs_spec.lua | 29 +--- 5 files changed, 404 insertions(+), 44 deletions(-) create mode 100644 test/unit/file_spec.lua diff --git a/src/nvim/file.c b/src/nvim/file.c index 2fb0e1cd87..2730e5b5f9 100644 --- a/src/nvim/file.c +++ b/src/nvim/file.c @@ -29,8 +29,6 @@ # include "file.c.generated.h" #endif -#define RWBUFSIZE (IOSIZE - 1) - /// Open file /// /// @param[out] ret_fp Address where information needed for reading from or @@ -80,7 +78,7 @@ int file_open(FileDescriptor *const ret_fp, const char *const fname, ret_fp->wr = (wr == kTrue); ret_fp->fd = fd; ret_fp->eof = false; - ret_fp->rv = rbuffer_new(RWBUFSIZE); + ret_fp->rv = rbuffer_new(kRWBufferSize); ret_fp->_error = 0; if (ret_fp->wr) { ret_fp->rv->data = ret_fp; @@ -162,7 +160,7 @@ int file_fsync(FileDescriptor *const fp) /// Buffer used for writing /// /// Like IObuff, but allows file_\* callers not to care about spoiling it. -static char writebuf[RWBUFSIZE]; +static char writebuf[kRWBufferSize]; /// Function run when RBuffer is full when writing to a file /// @@ -175,7 +173,10 @@ static void file_rb_write_full_cb(RBuffer *const rv, FileDescriptor *const fp) { assert(fp->wr); assert(rv->data == (void *)fp); - const size_t read_bytes = rbuffer_read(rv, writebuf, RWBUFSIZE); + if (rbuffer_size(rv) == 0) { + return; + } + const size_t read_bytes = rbuffer_read(rv, writebuf, kRWBufferSize); const ptrdiff_t wres = os_write(fp->fd, writebuf, read_bytes); if (wres != (ptrdiff_t)read_bytes) { if (wres >= 0) { @@ -215,22 +216,25 @@ ptrdiff_t file_read(FileDescriptor *const fp, char *const ret_buf, if (read_remaining) { assert(rbuffer_size(rv) == 0); rbuffer_reset(rv); - if (read_remaining >= RWBUFSIZE) { + if (read_remaining >= kRWBufferSize) { #ifdef HAVE_READV // If there is readv() syscall, then take an opportunity to populate // both target buffer and RBuffer at once, … - size_t read_count; + size_t write_count; struct iovec iov[] = { { .iov_base = buf, .iov_len = read_remaining }, - { .iov_base = rbuffer_read_ptr(rv, &read_count), .iov_len = RWBUFSIZE - }, + { .iov_base = rbuffer_write_ptr(rv, &write_count), + .iov_len = kRWBufferSize }, }; - assert(read_count == RWBUFSIZE); + assert(write_count == kRWBufferSize); const ptrdiff_t r_ret = os_readv(fp->fd, &fp->eof, iov, ARRAY_SIZE(iov)); if (r_ret > 0) { if (r_ret > (ptrdiff_t)read_remaining) { + read_remaining = 0; rbuffer_produced(rv, (size_t)(r_ret - (ptrdiff_t)read_remaining)); + } else { + read_remaining -= (size_t)r_ret; } } else if (r_ret < 0) { return r_ret; @@ -250,8 +254,8 @@ ptrdiff_t file_read(FileDescriptor *const fp, char *const ret_buf, size_t write_count; const ptrdiff_t r_ret = os_read(fp->fd, &fp->eof, rbuffer_write_ptr(rv, &write_count), - RWBUFSIZE); - assert(write_count == RWBUFSIZE); + kRWBufferSize); + assert(write_count == kRWBufferSize); if (r_ret > 0) { rbuffer_produced(rv, (size_t)r_ret); } else if (r_ret < 0) { @@ -288,7 +292,7 @@ ptrdiff_t file_write(FileDescriptor *const fp, const char *const buf, /// Buffer used for skipping. Its contents is undefined and should never be /// used. -static char skipbuf[RWBUFSIZE]; +static char skipbuf[kRWBufferSize]; /// Skip some bytes /// @@ -300,10 +304,8 @@ ptrdiff_t file_skip(FileDescriptor *const fp, const size_t size) assert(!fp->wr); size_t read_bytes = 0; do { - ptrdiff_t new_read_bytes = file_read( - fp, skipbuf, (size_t)(size - read_bytes > sizeof(skipbuf) - ? sizeof(skipbuf) - : size - read_bytes)); + const ptrdiff_t new_read_bytes = file_read( + fp, skipbuf, MIN(size - read_bytes, sizeof(skipbuf))); if (new_read_bytes < 0) { return new_read_bytes; } else if (new_read_bytes == 0) { diff --git a/src/nvim/file.h b/src/nvim/file.h index 5ee572750d..2ba415d2b9 100644 --- a/src/nvim/file.h +++ b/src/nvim/file.h @@ -32,6 +32,9 @@ typedef enum { ///< kFileCreateOnly. } FileOpenFlags; +static inline bool file_eof(const FileDescriptor *const fp) + REAL_FATTR_PURE REAL_FATTR_WARN_UNUSED_RESULT REAL_FATTR_NONNULL_ALL; + /// Check whether end of file was encountered /// /// @param[in] fp File to check. @@ -39,24 +42,30 @@ typedef enum { /// @return true if it was, false if it was not or read operation was never /// performed. static inline bool file_eof(const FileDescriptor *const fp) - FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ALL - FUNC_ATTR_ALWAYS_INLINE { return fp->eof && rbuffer_size(fp->rv) == 0; } +static inline int file_fd(const FileDescriptor *const fp) + REAL_FATTR_PURE REAL_FATTR_WARN_UNUSED_RESULT REAL_FATTR_NONNULL_ALL; + /// Return the file descriptor associated with the FileDescriptor structure /// /// @param[in] fp File to check. /// /// @return File descriptor. static inline int file_fd(const FileDescriptor *const fp) - FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ALL - FUNC_ATTR_ALWAYS_INLINE { return fp->fd; } +enum { + /// Read or write buffer size + /// + /// Currently equal to (IOSIZE - 1), but they do not need to be connected. + kRWBufferSize = 1024 +}; + #ifdef INCLUDE_GENERATED_DECLARATIONS # include "file.h.generated.h" #endif diff --git a/test/unit/file_spec.lua b/test/unit/file_spec.lua new file mode 100644 index 0000000000..ac78dffe6e --- /dev/null +++ b/test/unit/file_spec.lua @@ -0,0 +1,361 @@ +local lfs = require('lfs') + +local helpers = require('test.unit.helpers') + +local eq = helpers.eq +local ok = helpers.ok +local ffi = helpers.ffi +local cimport = helpers.cimport + +local m = cimport('./src/nvim/file.h') + +local s = '' +for i = 0, 255 do + s = s .. (i == 0 and '\0' or ('%c'):format(i)) +end +local fcontents = s:rep(16) + +local dir = 'Xtest-unit-file_spec.d' +local file1 = dir .. '/file1.dat' +local file2 = dir .. '/file2.dat' +local linkf = dir .. '/file.lnk' +local linkb = dir .. '/broken.lnk' +local filec = dir .. '/created-file.dat' + +before_each(function() + lfs.mkdir(dir); + + local f1 = io.open(file1, 'w') + f1:write(fcontents) + f1:close() + + local f2 = io.open(file2, 'w') + f2:write(fcontents) + f2:close() + + lfs.link('file1.dat', linkf, true) + lfs.link('broken.dat', linkb, true) +end) + +after_each(function() + os.remove(file1) + os.remove(file2) + os.remove(linkf) + os.remove(linkb) + os.remove(filec) + lfs.rmdir(dir) +end) + +local function file_open(fname, flags, mode) + local ret2 = ffi.new('FileDescriptor') + local ret1 = m.file_open(ret2, fname, flags, mode) + return ret1, ret2 +end + +local function file_open_new(fname, flags, mode) + local ret1 = ffi.new('int[?]', 1, {0}) + local ret2 = ffi.gc(m.file_open_new(ret1, fname, flags, mode), nil) + return ret1[0], ret2 +end + +local function file_write(fp, buf) + return m.file_write(fp, buf, #buf) +end + +local function file_read(fp, size) + local buf = nil + if size == nil then + size = 0 + else + buf = ffi.new('char[?]', size, ('\0'):rep(size)) + end + local ret1 = m.file_read(fp, buf, size) + local ret2 = '' + if buf ~= nil then + ret2 = ffi.string(buf, size) + end + return ret1, ret2 +end + +local function file_fsync(fp) + return m.file_fsync(fp) +end + +local function file_skip(fp, size) + return m.file_skip(fp, size) +end + +describe('file_open', function() + it('can create a rwx------ file with kFileCreate', function() + local err, fp = file_open(filec, m.kFileCreate, 448) + eq(0, err) + local attrs = lfs.attributes(filec) + eq('rwx------', attrs.permissions) + eq(0, m.file_close(fp)) + end) + + it('can create a rw------- file with kFileCreate', function() + local err, fp = file_open(filec, m.kFileCreate, 384) + eq(0, err) + local attrs = lfs.attributes(filec) + eq('rw-------', attrs.permissions) + eq(0, m.file_close(fp)) + end) + + it('can create a rwx------ file with kFileCreateOnly', function() + local err, fp = file_open(filec, m.kFileCreateOnly, 448) + eq(0, err) + local attrs = lfs.attributes(filec) + eq('rwx------', attrs.permissions) + eq(0, m.file_close(fp)) + end) + + it('can create a rw------- file with kFileCreateOnly', function() + local err, fp = file_open(filec, m.kFileCreateOnly, 384) + eq(0, err) + local attrs = lfs.attributes(filec) + eq('rw-------', attrs.permissions) + eq(0, m.file_close(fp)) + end) + + it('fails to open an existing file with kFileCreateOnly', function() + local err, fp = file_open(file1, m.kFileCreateOnly, 384) + eq(m.UV_EEXIST, err) + end) + + it('fails to open an symlink with kFileNoSymlink', function() + local err, fp = file_open(linkf, m.kFileNoSymlink, 384) + eq(m.UV_ELOOP, err) + end) + + it('can open an existing file write-only with kFileCreate', function() + local err, fp = file_open(file1, m.kFileCreate, 384) + eq(0, err) + eq(true, fp.wr) + eq(0, m.file_close(fp)) + end) + + it('can open an existing file read-only with zero', function() + local err, fp = file_open(file1, 0, 384) + eq(0, err) + eq(false, fp.wr) + eq(0, m.file_close(fp)) + end) + + it('can open an existing file read-only with kFileReadOnly', function() + local err, fp = file_open(file1, m.kFileReadOnly, 384) + eq(0, err) + eq(false, fp.wr) + eq(0, m.file_close(fp)) + end) + + it('can open an existing file read-only with kFileNoSymlink', function() + local err, fp = file_open(file1, m.kFileNoSymlink, 384) + eq(0, err) + eq(false, fp.wr) + eq(0, m.file_close(fp)) + end) + + it('can truncate an existing file with kFileTruncate', function() + local err, fp = file_open(file1, m.kFileTruncate, 384) + eq(0, err) + eq(true, fp.wr) + eq(0, m.file_close(fp)) + local attrs = lfs.attributes(file1) + eq(0, attrs.size) + end) + + it('can open an existing file write-only with kFileWriteOnly', function() + local err, fp = file_open(file1, m.kFileWriteOnly, 384) + eq(0, err) + eq(true, fp.wr) + eq(0, m.file_close(fp)) + local attrs = lfs.attributes(file1) + eq(4096, attrs.size) + end) + + it('fails to create a file with just kFileWriteOnly', function() + local err, fp = file_open(filec, m.kFileWriteOnly, 384) + eq(m.UV_ENOENT, err) + local attrs = lfs.attributes(filec) + eq(nil, attrs) + end) + + it('can truncate an existing file with kFileTruncate when opening a symlink', + function() + local err, fp = file_open(linkf, m.kFileTruncate, 384) + eq(0, err) + eq(true, fp.wr) + eq(0, m.file_close(fp)) + local attrs = lfs.attributes(file1) + eq(0, attrs.size) + end) + + it('fails to open a directory write-only', function() + local err, fp = file_open(dir, m.kFileWriteOnly, 384) + eq(m.UV_EISDIR, err) + end) + + it('fails to open a broken symbolic link write-only', function() + local err, fp = file_open(linkb, m.kFileWriteOnly, 384) + eq(m.UV_ENOENT, err) + end) + + it('fails to open a broken symbolic link read-only', function() + local err, fp = file_open(linkb, m.kFileReadOnly, 384) + eq(m.UV_ENOENT, err) + end) +end) + +describe('file_open_new', function() + it('can open a file read-only', function() + local err, fp = file_open_new(file1, 0, 384) + eq(0, err) + eq(false, fp.wr) + eq(0, m.file_free(fp)) + end) + + it('fails to open an existing file with kFileCreateOnly', function() + local err, fp = file_open_new(file1, m.kFileCreateOnly, 384) + eq(m.UV_EEXIST, err) + eq(nil, fp) + end) +end) + +-- file_close is called above, so it is not tested directly + +describe('file_fsync', function() + it('can flush writes to disk', function() + local err, fp = file_open(filec, m.kFileCreateOnly, 384) + eq(0, file_fsync(fp)) + eq(0, err) + eq(0, lfs.attributes(filec).size) + local wsize = file_write(fp, 'test') + eq(4, wsize) + eq(0, lfs.attributes(filec).size) + eq(0, file_fsync(fp)) + eq(wsize, lfs.attributes(filec).size) + eq(0, m.file_close(fp)) + end) +end) + +describe('file_read', function() + it('can read small chunks of input until eof', function() + local err, fp = file_open(file1, 0, 384) + eq(0, err) + eq(false, fp.wr) + local shift = 0 + while shift < #fcontents do + local size = 3 + local exp_err = size + local exp_s = fcontents:sub(shift + 1, shift + size) + if shift + size >= #fcontents then + exp_err = #fcontents - shift + exp_s = (fcontents:sub(shift + 1, shift + size) + .. (('\0'):rep(size - exp_err))) + end + eq({exp_err, exp_s}, {file_read(fp, size)}) + shift = shift + size + end + eq(0, m.file_close(fp)) + end) + + it('can read the whole file at once', function() + local err, fp = file_open(file1, 0, 384) + eq(0, err) + eq(false, fp.wr) + eq({#fcontents, fcontents}, {file_read(fp, #fcontents)}) + eq({0, ('\0'):rep(#fcontents)}, {file_read(fp, #fcontents)}) + eq(0, m.file_close(fp)) + end) + + it('can read more then 1024 bytes after reading a small chunk', function() + local err, fp = file_open(file1, 0, 384) + eq(0, err) + eq(false, fp.wr) + eq({5, fcontents:sub(1, 5)}, {file_read(fp, 5)}) + eq({#fcontents - 5, fcontents:sub(6) .. (('\0'):rep(5))}, + {file_read(fp, #fcontents)}) + eq(0, m.file_close(fp)) + end) + + it('can read file by 768-byte-chunks', function() + local err, fp = file_open(file1, 0, 384) + eq(0, err) + eq(false, fp.wr) + local shift = 0 + while shift < #fcontents do + local size = 768 + local exp_err = size + local exp_s = fcontents:sub(shift + 1, shift + size) + if shift + size >= #fcontents then + exp_err = #fcontents - shift + exp_s = (fcontents:sub(shift + 1, shift + size) + .. (('\0'):rep(size - exp_err))) + end + eq({exp_err, exp_s}, {file_read(fp, size)}) + shift = shift + size + end + eq(0, m.file_close(fp)) + end) +end) + +describe('file_write', function() + it('can write the whole file at once', function() + local err, fp = file_open(filec, m.kFileCreateOnly, 384) + eq(0, err) + eq(true, fp.wr) + local err = file_write(fp, fcontents) + eq(#fcontents, err) + eq(0, m.file_close(fp)) + eq(err, lfs.attributes(filec).size) + eq(fcontents, io.open(filec):read('*a')) + end) + + it('can write the whole file by small chunks', function() + local err, fp = file_open(filec, m.kFileCreateOnly, 384) + eq(0, err) + eq(true, fp.wr) + local shift = 0 + while shift < #fcontents do + local size = 3 + local s = fcontents:sub(shift + 1, shift + size) + local err = file_write(fp, s) + eq(err, #s) + shift = shift + size + end + eq(0, m.file_close(fp)) + eq(#fcontents, lfs.attributes(filec).size) + eq(fcontents, io.open(filec):read('*a')) + end) + + it('can write the whole file by 768-byte-chunks', function() + local err, fp = file_open(filec, m.kFileCreateOnly, 384) + eq(0, err) + eq(true, fp.wr) + local shift = 0 + while shift < #fcontents do + local size = 768 + local s = fcontents:sub(shift + 1, shift + size) + local err = file_write(fp, s) + eq(err, #s) + shift = shift + size + end + eq(0, m.file_close(fp)) + eq(#fcontents, lfs.attributes(filec).size) + eq(fcontents, io.open(filec):read('*a')) + end) +end) + +describe('file_skip', function() + it('can skip 3 bytes', function() + local err, fp = file_open(file1, 0, 384) + eq(0, err) + eq(false, fp.wr) + eq(3, file_skip(fp, 3)) + local err, s = file_read(fp, 3) + eq(3, err) + eq(fcontents:sub(4, 6), s) + eq(0, m.file_close(fp)) + end) +end) diff --git a/test/unit/helpers.lua b/test/unit/helpers.lua index f0c193884e..91da459393 100644 --- a/test/unit/helpers.lua +++ b/test/unit/helpers.lua @@ -7,6 +7,7 @@ local global_helpers = require('test.helpers') local neq = global_helpers.neq local eq = global_helpers.eq +local ok = global_helpers.ok -- add some standard header locations for _, p in ipairs(Paths.include_paths) do @@ -34,7 +35,8 @@ local function filter_complex_blocks(body) if not (string.find(line, "(^)", 1, true) ~= nil or string.find(line, "_ISwupper", 1, true) or string.find(line, "msgpack_zone_push_finalizer") - or string.find(line, "msgpack_unpacker_reserve_buffer")) then + or string.find(line, "msgpack_unpacker_reserve_buffer") + or string.find(line, "inline _Bool")) then result[#result + 1] = line end end @@ -156,6 +158,7 @@ return { cimport = cimport, cppimport = cppimport, internalize = internalize, + ok = ok, eq = eq, neq = neq, ffi = ffi, diff --git a/test/unit/os/fs_spec.lua b/test/unit/os/fs_spec.lua index d7e98b43ce..1f7f6fb791 100644 --- a/test/unit/os/fs_spec.lua +++ b/test/unit/os/fs_spec.lua @@ -6,6 +6,7 @@ local helpers = require('test.unit.helpers') local cimport = helpers.cimport local cppimport = helpers.cppimport local internalize = helpers.internalize +local ok = helpers.ok local eq = helpers.eq local neq = helpers.neq local ffi = helpers.ffi @@ -17,10 +18,6 @@ local NULL = helpers.NULL local NODE_NORMAL = 0 local NODE_WRITABLE = 1 -local function ok(expr) - assert.is_true(expr) -end - cimport('unistd.h') cimport('./src/nvim/os/shell.h') cimport('./src/nvim/option_defs.h') @@ -31,6 +28,12 @@ cppimport('sys/stat.h') cppimport('fcntl.h') cppimport('uv-errno.h') +local s = '' +for i = 0, 255 do + s = s .. (i == 0 and '\0' or ('%c'):format(i)) +end +local fcontents = s:rep(16) + local buffer = "" local directory = nil local absolute_executable = nil @@ -547,12 +550,6 @@ describe('fs function', function() describe('os_read', function() local file = 'test-unit-os-fs_spec-os_read.dat' - local s = '' - for i = 0, 255 do - s = s .. (i == 0 and '\0' or ('%c'):format(i)) - end - local fcontents = s:rep(16) - before_each(function() local f = io.open(file, 'w') f:write(fcontents) @@ -607,12 +604,6 @@ describe('fs function', function() end local file = 'test-unit-os-fs_spec-os_readv.dat' - local s = '' - for i = 0, 255 do - s = s .. (i == 0 and '\0' or ('%c'):format(i)) - end - local fcontents = s:rep(16) - before_each(function() local f = io.open(file, 'w') f:write(fcontents) @@ -673,12 +664,6 @@ describe('fs function', function() -- Function may be absent local file = 'test-unit-os-fs_spec-os_write.dat' - local s = '' - for i = 0, 255 do - s = s .. (i == 0 and '\0' or ('%c'):format(i)) - end - local fcontents = s:rep(16) - before_each(function() local f = io.open(file, 'w') f:write(fcontents) From 2dd154457ceb4bbc122b9e1ad34bf693ee3dd510 Mon Sep 17 00:00:00 2001 From: ZyX Date: Thu, 23 Jun 2016 21:17:24 +0300 Subject: [PATCH 08/13] file: Move src/nvim/file.* to src/nvim/os/fileio.* --- src/nvim/{file.c => os/fileio.c} | 6 +++--- src/nvim/{file.h => os/fileio.h} | 8 ++++---- src/nvim/shada.c | 2 +- test/unit/{file_spec.lua => os/fileio_spec.lua} | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) rename src/nvim/{file.c => os/fileio.c} (99%) rename src/nvim/{file.h => os/fileio.h} (95%) rename test/unit/{file_spec.lua => os/fileio_spec.lua} (99%) diff --git a/src/nvim/file.c b/src/nvim/os/fileio.c similarity index 99% rename from src/nvim/file.c rename to src/nvim/os/fileio.c index 2730e5b5f9..6abcceb485 100644 --- a/src/nvim/file.c +++ b/src/nvim/os/fileio.c @@ -1,4 +1,4 @@ -/// @file file.c +/// @file fileio.c /// /// Buffered reading/writing to a file. Unlike fileio.c this is not dealing with /// Neovim stuctures for buffer, with autocommands, etc: just fopen/fread/fwrite @@ -18,7 +18,7 @@ #include -#include "nvim/file.h" +#include "nvim/os/fileio.h" #include "nvim/memory.h" #include "nvim/os/os.h" #include "nvim/globals.h" @@ -26,7 +26,7 @@ #include "nvim/macros.h" #ifdef INCLUDE_GENERATED_DECLARATIONS -# include "file.c.generated.h" +# include "os/fileio.c.generated.h" #endif /// Open file diff --git a/src/nvim/file.h b/src/nvim/os/fileio.h similarity index 95% rename from src/nvim/file.h rename to src/nvim/os/fileio.h index 2ba415d2b9..2cffd5c851 100644 --- a/src/nvim/file.h +++ b/src/nvim/os/fileio.h @@ -1,5 +1,5 @@ -#ifndef NVIM_FILE_H -#define NVIM_FILE_H +#ifndef NVIM_OS_FILEIO_H +#define NVIM_OS_FILEIO_H #include #include @@ -67,6 +67,6 @@ enum { }; #ifdef INCLUDE_GENERATED_DECLARATIONS -# include "file.h.generated.h" +# include "os/fileio.h.generated.h" #endif -#endif // NVIM_FILE_H +#endif // NVIM_OS_FILEIO_H diff --git a/src/nvim/shada.c b/src/nvim/shada.c index a833b958b9..5047ef9647 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -35,7 +35,7 @@ #include "nvim/version.h" #include "nvim/path.h" #include "nvim/fileio.h" -#include "nvim/file.h" +#include "nvim/os/fileio.h" #include "nvim/strings.h" #include "nvim/quickfix.h" #include "nvim/eval/encode.h" diff --git a/test/unit/file_spec.lua b/test/unit/os/fileio_spec.lua similarity index 99% rename from test/unit/file_spec.lua rename to test/unit/os/fileio_spec.lua index ac78dffe6e..22b61786c0 100644 --- a/test/unit/file_spec.lua +++ b/test/unit/os/fileio_spec.lua @@ -7,7 +7,7 @@ local ok = helpers.ok local ffi = helpers.ffi local cimport = helpers.cimport -local m = cimport('./src/nvim/file.h') +local m = cimport('./src/nvim/os/fileio.h') local s = '' for i = 0, 255 do From a3b05a03b62b2c5be92a63ba3890b0359a335ba7 Mon Sep 17 00:00:00 2001 From: ZyX Date: Thu, 23 Jun 2016 22:25:35 +0300 Subject: [PATCH 09/13] unittests: Fix bug somewhere that makes file_read tests SEGV --- test/unit/os/fileio_spec.lua | 5 ++++- test/unit/os/fs_spec.lua | 8 ++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/test/unit/os/fileio_spec.lua b/test/unit/os/fileio_spec.lua index 22b61786c0..3f0bd16e6c 100644 --- a/test/unit/os/fileio_spec.lua +++ b/test/unit/os/fileio_spec.lua @@ -67,7 +67,10 @@ local function file_read(fp, size) if size == nil then size = 0 else - buf = ffi.new('char[?]', size, ('\0'):rep(size)) + -- For some reason if length of NUL-bytes-string is the same as `char[?]` + -- size luajit garbage collector crashes. But it does not do so in + -- os_read[v] tests in os/fs_spec.lua. + buf = ffi.new('char[?]', size + 1, ('\0'):rep(size)) end local ret1 = m.file_read(fp, buf, size) local ret2 = '' diff --git a/test/unit/os/fs_spec.lua b/test/unit/os/fs_spec.lua index 1f7f6fb791..cc10b0cfa4 100644 --- a/test/unit/os/fs_spec.lua +++ b/test/unit/os/fs_spec.lua @@ -378,12 +378,16 @@ describe('fs function', function() local function os_close(fd) return fs.os_close(fd) end + -- For some reason if length of NUL-bytes-string is the same as `char[?]` + -- size luajit crashes. Though it does not do so in this test suite, better + -- be cautios and allocate more elements then needed. I only did this to + -- strings. local function os_read(fd, size) local buf = nil if size == nil then size = 0 else - buf = ffi.new('char[?]', size, ('\0'):rep(size)) + buf = ffi.new('char[?]', size + 1, ('\0'):rep(size)) end local eof = ffi.new('bool[?]', 1, {true}) local ret2 = fs.os_read(fd, eof, buf, size) @@ -398,7 +402,7 @@ describe('fs function', function() local bufs = {} for i, size in ipairs(sizes) do bufs[i] = { - iov_base=ffi.new('char[?]', size, ('\0'):rep(size)), + iov_base=ffi.new('char[?]', size + 1, ('\0'):rep(size)), iov_len=size, } end From 75bd39d49c09896a885ac700862f9b3bb84f6069 Mon Sep 17 00:00:00 2001 From: ZyX Date: Thu, 23 Jun 2016 23:49:23 +0300 Subject: [PATCH 10/13] *: Satisfy linter (newest type casts rule) --- src/nvim/os/fs.c | 14 +++++++------- src/nvim/shada.c | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/nvim/os/fs.c b/src/nvim/os/fs.c index 19d4cbc440..49c42cb63d 100644 --- a/src/nvim/os/fs.c +++ b/src/nvim/os/fs.c @@ -392,7 +392,7 @@ ptrdiff_t os_read(const int fd, bool *ret_eof, char *const ret_buf, const ptrdiff_t cur_read_bytes = read(fd, ret_buf + read_bytes, size - read_bytes); if (cur_read_bytes > 0) { - read_bytes += (size_t) cur_read_bytes; + read_bytes += (size_t)cur_read_bytes; assert(read_bytes <= size); } if (cur_read_bytes < 0) { @@ -412,7 +412,7 @@ ptrdiff_t os_read(const int fd, bool *ret_eof, char *const ret_buf, did_try_to_free = true; continue; } else { - return (ptrdiff_t) error; + return (ptrdiff_t)error; } } if (cur_read_bytes == 0) { @@ -420,11 +420,11 @@ ptrdiff_t os_read(const int fd, bool *ret_eof, char *const ret_buf, break; } } - return (ptrdiff_t) read_bytes; + return (ptrdiff_t)read_bytes; } #ifdef HAVE_READV -/// Read from a file to a multiple buffers at once +/// Read from a file to multiple buffers at once /// /// Wrapper for readv(). /// @@ -455,7 +455,7 @@ ptrdiff_t os_readv(int fd, bool *ret_eof, struct iovec *iov, size_t iov_size) if (cur_read_bytes > 0) { read_bytes += (size_t)cur_read_bytes; while (iov_size && cur_read_bytes) { - if (cur_read_bytes < (ptrdiff_t) iov->iov_len) { + if (cur_read_bytes < (ptrdiff_t)iov->iov_len) { iov->iov_len -= (size_t)cur_read_bytes; iov->iov_base = (char *)iov->iov_base + cur_read_bytes; cur_read_bytes = 0; @@ -509,7 +509,7 @@ ptrdiff_t os_write(const int fd, const char *const buf, const size_t size) const ptrdiff_t cur_written_bytes = write(fd, buf + written_bytes, size - written_bytes); if (cur_written_bytes > 0) { - written_bytes += (size_t) cur_written_bytes; + written_bytes += (size_t)cur_written_bytes; } if (cur_written_bytes < 0) { #ifdef HAVE_UV_TRANSLATE_SYS_ERROR @@ -532,7 +532,7 @@ ptrdiff_t os_write(const int fd, const char *const buf, const size_t size) return UV_UNKNOWN; } } - return (ptrdiff_t) written_bytes; + return (ptrdiff_t)written_bytes; } /// Flushes file modifications to disk. diff --git a/src/nvim/shada.c b/src/nvim/shada.c index 5047ef9647..b5921eb810 100644 --- a/src/nvim/shada.c +++ b/src/nvim/shada.c @@ -3007,8 +3007,8 @@ shada_write_file_nomerge: {} if (getuid() == ROOT_UID) { if (old_info.stat.st_uid != ROOT_UID || old_info.stat.st_gid != getgid()) { - const uv_uid_t old_uid = (uv_uid_t) old_info.stat.st_uid; - const uv_gid_t old_gid = (uv_gid_t) old_info.stat.st_gid; + const uv_uid_t old_uid = (uv_uid_t)old_info.stat.st_uid; + const uv_gid_t old_gid = (uv_gid_t)old_info.stat.st_gid; const int fchown_ret = os_fchown(file_fd(sd_writer.cookie), old_uid, old_gid); if (fchown_ret != 0) { @@ -3192,7 +3192,7 @@ static ShaDaReadResult fread_len(ShaDaReadDef *const sd_reader, emsgf(_(RCERR "Error while reading ShaDa file: " "last entry specified that it occupies %" PRIu64 " bytes, " "but file ended earlier"), - (uint64_t) length); + (uint64_t)length); return kSDReadStatusNotShaDa; } } From 6580dfeddd9693b778f59ca5c333d05200d67687 Mon Sep 17 00:00:00 2001 From: ZyX Date: Fri, 24 Jun 2016 00:06:46 +0300 Subject: [PATCH 11/13] unittests: Fix kFileNoSymlink test on FreeBSD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Actual value on FreeBSD is -31, UV_EMLINK was obtained from /usr/include/asm-generic/errno-base.h (there EMLINK is defined as 31 there). This may actually be something else, but I do not think so as “Too many links” description also fits in. [Man page][1] agrees with me, search for `[EMLINK]` ([linux man page][2] also specifies ELOOP explicitly in a similar section). [1]: https://www.freebsd.org/cgi/man.cgi?query=open&sektion=2 [2]: http://man7.org/linux/man-pages/man3/open.3p.html --- test/unit/os/fileio_spec.lua | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/os/fileio_spec.lua b/test/unit/os/fileio_spec.lua index 3f0bd16e6c..59d85926d9 100644 --- a/test/unit/os/fileio_spec.lua +++ b/test/unit/os/fileio_spec.lua @@ -128,7 +128,9 @@ describe('file_open', function() it('fails to open an symlink with kFileNoSymlink', function() local err, fp = file_open(linkf, m.kFileNoSymlink, 384) - eq(m.UV_ELOOP, err) + -- err is UV_EMLINK in FreeBSD, but if I use `ok(err == m.UV_ELOOP or err == + -- m.UV_EMLINK)`, then I loose the ability to see actual `err` value. + if err ~= m.UV_ELOOP then eq(m.UV_EMLINK, err) end end) it('can open an existing file write-only with kFileCreate', function() From 96a57e1bc66f294684e9ac2ee8f2b74243982123 Mon Sep 17 00:00:00 2001 From: ZyX Date: Fri, 24 Jun 2016 01:07:09 +0300 Subject: [PATCH 12/13] os/fileio: Use readv often --- src/nvim/os/fileio.c | 45 ++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/src/nvim/os/fileio.c b/src/nvim/os/fileio.c index 6abcceb485..6cee102305 100644 --- a/src/nvim/os/fileio.c +++ b/src/nvim/os/fileio.c @@ -216,30 +216,31 @@ ptrdiff_t file_read(FileDescriptor *const fp, char *const ret_buf, if (read_remaining) { assert(rbuffer_size(rv) == 0); rbuffer_reset(rv); - if (read_remaining >= kRWBufferSize) { #ifdef HAVE_READV - // If there is readv() syscall, then take an opportunity to populate - // both target buffer and RBuffer at once, … - size_t write_count; - struct iovec iov[] = { - { .iov_base = buf, .iov_len = read_remaining }, - { .iov_base = rbuffer_write_ptr(rv, &write_count), - .iov_len = kRWBufferSize }, - }; - assert(write_count == kRWBufferSize); - const ptrdiff_t r_ret = os_readv(fp->fd, &fp->eof, iov, - ARRAY_SIZE(iov)); - if (r_ret > 0) { - if (r_ret > (ptrdiff_t)read_remaining) { - read_remaining = 0; - rbuffer_produced(rv, (size_t)(r_ret - (ptrdiff_t)read_remaining)); - } else { - read_remaining -= (size_t)r_ret; - } - } else if (r_ret < 0) { - return r_ret; + // If there is readv() syscall, then take an opportunity to populate + // both target buffer and RBuffer at once, … + size_t write_count; + struct iovec iov[] = { + { .iov_base = buf, .iov_len = read_remaining }, + { .iov_base = rbuffer_write_ptr(rv, &write_count), + .iov_len = kRWBufferSize }, + }; + assert(write_count == kRWBufferSize); + const ptrdiff_t r_ret = os_readv(fp->fd, &fp->eof, iov, + ARRAY_SIZE(iov)); + if (r_ret > 0) { + if (r_ret > (ptrdiff_t)read_remaining) { + rbuffer_produced(rv, (size_t)(r_ret - (ptrdiff_t)read_remaining)); + read_remaining = 0; + } else { + buf += (size_t)r_ret; + read_remaining -= (size_t)r_ret; } + } else if (r_ret < 0) { + return r_ret; + } #else + if (read_remaining >= kRWBufferSize) { // …otherwise leave RBuffer empty and populate only target buffer, // because filtering information through rbuffer will be more syscalls. const ptrdiff_t r_ret = os_read(fp->fd, &fp->eof, buf, read_remaining); @@ -249,7 +250,6 @@ ptrdiff_t file_read(FileDescriptor *const fp, char *const ret_buf, } else if (r_ret < 0) { return r_ret; } -#endif } else { size_t write_count; const ptrdiff_t r_ret = os_read(fp->fd, &fp->eof, @@ -262,6 +262,7 @@ ptrdiff_t file_read(FileDescriptor *const fp, char *const ret_buf, return r_ret; } } +#endif } } return (ptrdiff_t)(size - read_remaining); From 4741c8b234eeaeac839b689d6316528ecc78a934 Mon Sep 17 00:00:00 2001 From: ZyX Date: Fri, 24 Jun 2016 01:12:30 +0300 Subject: [PATCH 13/13] unittests: Fix testlint errors --- test/unit/os/fileio_spec.lua | 37 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/test/unit/os/fileio_spec.lua b/test/unit/os/fileio_spec.lua index 59d85926d9..5358022422 100644 --- a/test/unit/os/fileio_spec.lua +++ b/test/unit/os/fileio_spec.lua @@ -3,17 +3,16 @@ local lfs = require('lfs') local helpers = require('test.unit.helpers') local eq = helpers.eq -local ok = helpers.ok local ffi = helpers.ffi local cimport = helpers.cimport local m = cimport('./src/nvim/os/fileio.h') -local s = '' +local fcontents = '' for i = 0, 255 do - s = s .. (i == 0 and '\0' or ('%c'):format(i)) + fcontents = fcontents .. (i == 0 and '\0' or ('%c'):format(i)) end -local fcontents = s:rep(16) +fcontents = fcontents:rep(16) local dir = 'Xtest-unit-file_spec.d' local file1 = dir .. '/file1.dat' @@ -122,12 +121,12 @@ describe('file_open', function() end) it('fails to open an existing file with kFileCreateOnly', function() - local err, fp = file_open(file1, m.kFileCreateOnly, 384) + local err, _ = file_open(file1, m.kFileCreateOnly, 384) eq(m.UV_EEXIST, err) end) it('fails to open an symlink with kFileNoSymlink', function() - local err, fp = file_open(linkf, m.kFileNoSymlink, 384) + local err, _ = file_open(linkf, m.kFileNoSymlink, 384) -- err is UV_EMLINK in FreeBSD, but if I use `ok(err == m.UV_ELOOP or err == -- m.UV_EMLINK)`, then I loose the ability to see actual `err` value. if err ~= m.UV_ELOOP then eq(m.UV_EMLINK, err) end @@ -180,7 +179,7 @@ describe('file_open', function() end) it('fails to create a file with just kFileWriteOnly', function() - local err, fp = file_open(filec, m.kFileWriteOnly, 384) + local err, _ = file_open(filec, m.kFileWriteOnly, 384) eq(m.UV_ENOENT, err) local attrs = lfs.attributes(filec) eq(nil, attrs) @@ -197,17 +196,17 @@ describe('file_open', function() end) it('fails to open a directory write-only', function() - local err, fp = file_open(dir, m.kFileWriteOnly, 384) + local err, _ = file_open(dir, m.kFileWriteOnly, 384) eq(m.UV_EISDIR, err) end) it('fails to open a broken symbolic link write-only', function() - local err, fp = file_open(linkb, m.kFileWriteOnly, 384) + local err, _ = file_open(linkb, m.kFileWriteOnly, 384) eq(m.UV_ENOENT, err) end) it('fails to open a broken symbolic link read-only', function() - local err, fp = file_open(linkb, m.kFileReadOnly, 384) + local err, _ = file_open(linkb, m.kFileReadOnly, 384) eq(m.UV_ENOENT, err) end) end) @@ -310,10 +309,10 @@ describe('file_write', function() local err, fp = file_open(filec, m.kFileCreateOnly, 384) eq(0, err) eq(true, fp.wr) - local err = file_write(fp, fcontents) - eq(#fcontents, err) + local wr = file_write(fp, fcontents) + eq(#fcontents, wr) eq(0, m.file_close(fp)) - eq(err, lfs.attributes(filec).size) + eq(wr, lfs.attributes(filec).size) eq(fcontents, io.open(filec):read('*a')) end) @@ -325,8 +324,8 @@ describe('file_write', function() while shift < #fcontents do local size = 3 local s = fcontents:sub(shift + 1, shift + size) - local err = file_write(fp, s) - eq(err, #s) + local wr = file_write(fp, s) + eq(wr, #s) shift = shift + size end eq(0, m.file_close(fp)) @@ -342,8 +341,8 @@ describe('file_write', function() while shift < #fcontents do local size = 768 local s = fcontents:sub(shift + 1, shift + size) - local err = file_write(fp, s) - eq(err, #s) + local wr = file_write(fp, s) + eq(wr, #s) shift = shift + size end eq(0, m.file_close(fp)) @@ -358,8 +357,8 @@ describe('file_skip', function() eq(0, err) eq(false, fp.wr) eq(3, file_skip(fp, 3)) - local err, s = file_read(fp, 3) - eq(3, err) + local rd, s = file_read(fp, 3) + eq(3, rd) eq(fcontents:sub(4, 6), s) eq(0, m.file_close(fp)) end)