Re: [dpdk-dev] [PATCH v6 02/11] eal: introduce internal wrappers for file operations

[Neil Horman <nhorman@tuxdriver.com>]

On Wed, Jun 03, 2020 at 02:03:20AM +0300, Dmitry Kozlyuk wrote:
> Introduce OS-independent wrappers in order to support common EAL code
> on Unix and Windows:
> 
> * eal_file_create: open an existing file.
> * eal_file_open: create a file or open it if exists.
> * eal_file_lock: lock or unlock an open file.
> * eal_file_truncate: enforce a given size for an open file.
> 
> Implementation for Linux and FreeBSD is placed in "unix" subdirectory,
> which is intended for common code between the two. These thin wrappers
> require no special maintenance.
> 
> Common code supporting multi-process doesn't use the new wrappers,
> because it is inherently Unix-specific and would impose excessive
> requirements on the wrappers.
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>  MAINTAINERS                                |  3 +
>  lib/librte_eal/common/eal_common_fbarray.c | 31 ++++-----
>  lib/librte_eal/common/eal_private.h        | 74 +++++++++++++++++++++
>  lib/librte_eal/freebsd/Makefile            |  4 ++
>  lib/librte_eal/linux/Makefile              |  4 ++
>  lib/librte_eal/meson.build                 |  4 ++
>  lib/librte_eal/unix/eal_file.c             | 76 ++++++++++++++++++++++
>  lib/librte_eal/unix/meson.build            |  6 ++
>  8 files changed, 183 insertions(+), 19 deletions(-)
>  create mode 100644 lib/librte_eal/unix/eal_file.c
>  create mode 100644 lib/librte_eal/unix/meson.build
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d2b286701..1d9aff26d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -323,6 +323,9 @@ FreeBSD UIO
>  M: Bruce Richardson <bruce.richardson@intel.com>
>  F: kernel/freebsd/nic_uio/
>  
> +Unix shared files
> +F: lib/librte_eal/unix/
> +
>  Windows support
>  M: Harini Ramakrishnan <harini.ramakrishnan@microsoft.com>
>  M: Omar Cardona <ocardona@microsoft.com>
> diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
> index 4f8f1af73..81ce4bd42 100644
> --- a/lib/librte_eal/common/eal_common_fbarray.c
> +++ b/lib/librte_eal/common/eal_common_fbarray.c
> @@ -8,8 +8,8 @@
>  #include <sys/mman.h>
>  #include <stdint.h>
>  #include <errno.h>
> -#include <sys/file.h>
>  #include <string.h>
> +#include <unistd.h>
>  
>  #include <rte_common.h>
>  #include <rte_log.h>
> @@ -85,10 +85,8 @@ resize_and_map(int fd, void *addr, size_t len)
>  	char path[PATH_MAX];
>  	void *map_addr;
>  
> -	if (ftruncate(fd, len)) {
> +	if (eal_file_truncate(fd, len)) {
>  		RTE_LOG(ERR, EAL, "Cannot truncate %s\n", path);
> -		/* pass errno up the chain */
> -		rte_errno = errno;
>  		return -1;
>  	}
>  
> @@ -772,15 +770,15 @@ rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,
>  		 * and see if we succeed. If we don't, someone else is using it
>  		 * already.
>  		 */
> -		fd = open(path, O_CREAT | O_RDWR, 0600);
> +		fd = eal_file_create(path);
>  		if (fd < 0) {
>  			RTE_LOG(DEBUG, EAL, "%s(): couldn't open %s: %s\n",
> -					__func__, path, strerror(errno));
> -			rte_errno = errno;
> +				__func__, path, rte_strerror(rte_errno));
>  			goto fail;
> -		} else if (flock(fd, LOCK_EX | LOCK_NB)) {
> +		} else if (eal_file_lock(
> +				fd, EAL_FLOCK_EXCLUSIVE, EAL_FLOCK_RETURN)) {
>  			RTE_LOG(DEBUG, EAL, "%s(): couldn't lock %s: %s\n",
> -					__func__, path, strerror(errno));
> +				__func__, path, rte_strerror(rte_errno));
>  			rte_errno = EBUSY;
>  			goto fail;
>  		}
> @@ -789,10 +787,8 @@ rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,
>  		 * still attach to it, but no other process could reinitialize
>  		 * it.
>  		 */
> -		if (flock(fd, LOCK_SH | LOCK_NB)) {
> -			rte_errno = errno;
> +		if (eal_file_lock(fd, EAL_FLOCK_SHARED, EAL_FLOCK_RETURN))
>  			goto fail;
> -		}
>  
>  		if (resize_and_map(fd, data, mmap_len))
>  			goto fail;
> @@ -888,17 +884,14 @@ rte_fbarray_attach(struct rte_fbarray *arr)
>  
>  	eal_get_fbarray_path(path, sizeof(path), arr->name);
>  
> -	fd = open(path, O_RDWR);
> +	fd = eal_file_open(path, true);
>  	if (fd < 0) {
> -		rte_errno = errno;
>  		goto fail;
>  	}
>  
>  	/* lock the file, to let others know we're using it */
> -	if (flock(fd, LOCK_SH | LOCK_NB)) {
> -		rte_errno = errno;
> +	if (eal_file_lock(fd, EAL_FLOCK_SHARED, EAL_FLOCK_RETURN))
>  		goto fail;
> -	}
>  
>  	if (resize_and_map(fd, data, mmap_len))
>  		goto fail;
> @@ -1025,7 +1018,7 @@ rte_fbarray_destroy(struct rte_fbarray *arr)
>  		 * has been detached by all other processes
>  		 */
>  		fd = tmp->fd;
> -		if (flock(fd, LOCK_EX | LOCK_NB)) {
> +		if (eal_file_lock(fd, EAL_FLOCK_EXCLUSIVE, EAL_FLOCK_RETURN)) {
>  			RTE_LOG(DEBUG, EAL, "Cannot destroy fbarray - another process is using it\n");
>  			rte_errno = EBUSY;
>  			ret = -1;
> @@ -1042,7 +1035,7 @@ rte_fbarray_destroy(struct rte_fbarray *arr)
>  			 * we're still holding an exclusive lock, so drop it to
>  			 * shared.
>  			 */
> -			flock(fd, LOCK_SH | LOCK_NB);
> +			eal_file_lock(fd, EAL_FLOCK_SHARED, EAL_FLOCK_RETURN);
>  
>  			ret = -1;
>  			goto out;
> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> index 869ce183a..727f26881 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -420,4 +420,78 @@ eal_malloc_no_trace(const char *type, size_t size, unsigned int align);
>  
>  void eal_free_no_trace(void *addr);
>  
> +/**
> + * Create a file or open it if exits.
> + *
> + * Newly created file is only accessible to the owner (0600 equivalent).
> + * Returned descriptor is always read/write.
> + *
> + * @param path
> + *  Path to the file.
> + * @return
> + *  Open file descriptor on success, (-1) on failure and rte_errno is set.
> + */
> +int
> +eal_file_create(const char *path);
> +
> +/**
> + * Open an existing file.
> + *
> + * @param path
> + *  Path to the file.
> + * @param writable
> + *  Whether to open file read/write or read-only.
> + * @return
> + *  Open file descriptor on success, (-1) on failure and rte_errno is set.
> + */
> +int
> +eal_file_open(const char *path, bool writable);
> +
> +/** File locking operation. */
> +enum eal_flock_op {
> +	EAL_FLOCK_SHARED,    /**< Acquire a shared lock. */
> +	EAL_FLOCK_EXCLUSIVE, /**< Acquire an exclusive lock. */
> +	EAL_FLOCK_UNLOCK     /**< Release a previously taken lock. */
> +};
> +
> +/** Behavior on file locking conflict. */
> +enum eal_flock_mode {
> +	EAL_FLOCK_WAIT,  /**< Wait until the file gets unlocked to lock it. */
> +	EAL_FLOCK_RETURN /**< Return immediately if the file is locked. */
> +};
> +
> +/**
> + * Lock or unlock the file.
> + *
> + * On failure @code rte_errno @endcode is set to the error code
> + * specified by POSIX flock(3) description.
> + *
> + * @param fd
> + *  Opened file descriptor.
> + * @param op
> + *  Operation to perform.
> + * @param mode
> + *  Behavior on conflict.
> + * @return
> + *  0 on success, (-1) on failure.
> + */
> +int
> +eal_file_lock(int fd, enum eal_flock_op op, enum eal_flock_mode mode);
> +
> +/**
> + * Truncate or extend the file to the specified size.
> + *
> + * On failure @code rte_errno @endcode is set to the error code
> + * specified by POSIX ftruncate(3) description.
> + *
> + * @param fd
> + *  Opened file descriptor.
> + * @param size
> + *  Desired file size.
> + * @return
> + *  0 on success, (-1) on failure.
> + */
> +int
> +eal_file_truncate(int fd, ssize_t size);
> +
>  #endif /* _EAL_PRIVATE_H_ */
> diff --git a/lib/librte_eal/freebsd/Makefile b/lib/librte_eal/freebsd/Makefile
> index af95386d4..0f8741d96 100644
> --- a/lib/librte_eal/freebsd/Makefile
> +++ b/lib/librte_eal/freebsd/Makefile
> @@ -7,6 +7,7 @@ LIB = librte_eal.a
>  
>  ARCH_DIR ?= $(RTE_ARCH)
>  VPATH += $(RTE_SDK)/lib/librte_eal/$(ARCH_DIR)
> +VPATH += $(RTE_SDK)/lib/librte_eal/unix
>  VPATH += $(RTE_SDK)/lib/librte_eal/common
>  
>  CFLAGS += -I$(SRCDIR)/include
> @@ -74,6 +75,9 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_service.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_random.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_reciprocal.c
>  
> +# from unix dir
> +SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_file.c
> +
>  # from arch dir
>  SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_cpuflags.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_hypervisor.c
> diff --git a/lib/librte_eal/linux/Makefile b/lib/librte_eal/linux/Makefile
> index 48cc34844..331489f99 100644
> --- a/lib/librte_eal/linux/Makefile
> +++ b/lib/librte_eal/linux/Makefile
> @@ -7,6 +7,7 @@ LIB = librte_eal.a
>  
>  ARCH_DIR ?= $(RTE_ARCH)
>  VPATH += $(RTE_SDK)/lib/librte_eal/$(ARCH_DIR)
> +VPATH += $(RTE_SDK)/lib/librte_eal/unix
>  VPATH += $(RTE_SDK)/lib/librte_eal/common
>  
>  CFLAGS += -I$(SRCDIR)/include
> @@ -81,6 +82,9 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_service.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_random.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_reciprocal.c
>  
> +# from unix dir
> +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_file.c
> +
>  # from arch dir
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_cpuflags.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_hypervisor.c
> diff --git a/lib/librte_eal/meson.build b/lib/librte_eal/meson.build
> index e301f4558..8d492897d 100644
> --- a/lib/librte_eal/meson.build
> +++ b/lib/librte_eal/meson.build
> @@ -6,6 +6,10 @@ subdir('include')
>  
>  subdir('common')
>  
> +if not is_windows
> +	subdir('unix')
> +endif
> +
>  dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
>  subdir(exec_env)
>  
> diff --git a/lib/librte_eal/unix/eal_file.c b/lib/librte_eal/unix/eal_file.c
> new file mode 100644
> index 000000000..7b3ffa629
> --- /dev/null
> +++ b/lib/librte_eal/unix/eal_file.c
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Dmitry Kozlyuk
> + */
> +
> +#include <sys/file.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +
> +#include <rte_errno.h>
> +
> +#include "eal_private.h"
> +
> +int
> +eal_file_create(const char *path)
> +{
> +	int ret;
> +
> +	ret = open(path, O_CREAT | O_RDWR, 0600);
> +	if (ret < 0)
> +		rte_errno = errno;
> +
> +	return ret;
> +}
> +
You don't need this call if you support the oflags option in the open call
below.

> +int
> +eal_file_open(const char *path, bool writable)
> +{
> +	int ret, flags;
> +
> +	flags = writable ? O_RDWR : O_RDONLY;
> +	ret = open(path, flags);
> +	if (ret < 0)
> +		rte_errno = errno;
> +
> +	return ret;
> +}
> +
why are you changing this api from the posix file format (with oflags
specified).  As far as I can see both unix and windows platforms support that

> +int
> +eal_file_truncate(int fd, ssize_t size)
> +{
> +	int ret;
> +
> +	ret = ftruncate(fd, size);
> +	if (ret)
> +		rte_errno = errno;
> +
> +	return ret;
> +}
> +
> +int
> +eal_file_lock(int fd, enum eal_flock_op op, enum eal_flock_mode mode)
> +{
> +	int sys_flags = 0;
> +	int ret;
> +
> +	if (mode == EAL_FLOCK_RETURN)
> +		sys_flags |= LOCK_NB;
> +
> +	switch (op) {
> +	case EAL_FLOCK_EXCLUSIVE:
> +		sys_flags |= LOCK_EX;
> +		break;
> +	case EAL_FLOCK_SHARED:
> +		sys_flags |= LOCK_SH;
> +		break;
> +	case EAL_FLOCK_UNLOCK:
> +		sys_flags |= LOCK_UN;
> +		break;
> +	}
> +
> +	ret = flock(fd, sys_flags);
> +	if (ret)
> +		rte_errno = errno;
> +
> +	return ret;
> +}
> diff --git a/lib/librte_eal/unix/meson.build b/lib/librte_eal/unix/meson.build
> new file mode 100644
> index 000000000..21029ba1a
> --- /dev/null
> +++ b/lib/librte_eal/unix/meson.build
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2020 Dmitry Kozlyuk
> +
> +sources += files(
> +	'eal_file.c',
> +)
> -- 
> 2.25.4
> 
>