[v6,02/11] eal: introduce internal wrappers for file operations
Checks
Commit Message
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
Comments
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
>
>
On Wed, 3 Jun 2020 08:07:59 -0400
Neil Horman <nhorman@tuxdriver.com> wrote:
[snip]
> > +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.
See 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
There is a number of caveats, which IMO make this approach better:
1. Filesystem permissions on Windows are complicated. Supporting anything
other than 0600 would add a lot of code, while EAL doesn't really need it.
Microsoft's open() takes not permission bits, but a set of flags.
2. Restricted interface prevents EAL developers from accidentally using
features not supported on all platforms via a seemingly rich API.
3. Microsoft CRT (the one Clang is using) deprecates open() in favor of
_sopen_s() and issues a warning, and we're targeting -Werror. Disabling all
such warnings (_CRT_SECURE_NO_DEPRECATE) doesn't seem right when CRT vendor
encourages using alternatives. This is the primary reason for open()
wrappers in v6.
On Wed, Jun 03, 2020 at 03:34:03PM +0300, Dmitry Kozlyuk wrote:
> On Wed, 3 Jun 2020 08:07:59 -0400
> Neil Horman <nhorman@tuxdriver.com> wrote:
>
> [snip]
> > > +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.
>
> See 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
>
> There is a number of caveats, which IMO make this approach better:
>
> 1. Filesystem permissions on Windows are complicated. Supporting anything
> other than 0600 would add a lot of code, while EAL doesn't really need it.
> Microsoft's open() takes not permission bits, but a set of flags.
>
> 2. Restricted interface prevents EAL developers from accidentally using
> features not supported on all platforms via a seemingly rich API.
>
> 3. Microsoft CRT (the one Clang is using) deprecates open() in favor of
> _sopen_s() and issues a warning, and we're targeting -Werror. Disabling all
> such warnings (_CRT_SECURE_NO_DEPRECATE) doesn't seem right when CRT vendor
> encourages using alternatives. This is the primary reason for open()
> wrappers in v6.
>
that seems a bit shortsighted to me. By creating wrappers that restrict
functionality to the least common demoninator of supported platforms restricts
what all platforms are capable of. For example, theres no reason that the eal
library shouldn't be able to open a file O_TRUNC or O_SYNC just because its
complex to do it on a single platform.
The API should be written to support the full range of functionality on all
platforms, and the individual implementations should write the code to make that
happen, or return an error that its unsupported on this particular platform.
I'm not saying that you have to implement everything now, but you shouldn't
restrict the API from being able to do so in the future. Otherwise, in the
future, if someone wants to implement O_TRUNC support (just to site an example),
they're going to have to make a change to the API above, and alter the
implementation for all the platforms anyway. You may as well make the API
robust enough to support that now.
Neil
> --
> Dmitry Kozlyuk
>
On Thu, 4 Jun 2020 17:07:07 -0400
Neil Horman <nhorman@tuxdriver.com> wrote:
> On Wed, Jun 03, 2020 at 03:34:03PM +0300, Dmitry Kozlyuk wrote:
> > On Wed, 3 Jun 2020 08:07:59 -0400
> > Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > [snip]
> > > > +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.
> >
> > See 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
> >
> > There is a number of caveats, which IMO make this approach better:
> >
> > 1. Filesystem permissions on Windows are complicated. Supporting anything
> > other than 0600 would add a lot of code, while EAL doesn't really need it.
> > Microsoft's open() takes not permission bits, but a set of flags.
> >
> > 2. Restricted interface prevents EAL developers from accidentally using
> > features not supported on all platforms via a seemingly rich API.
> >
> > 3. Microsoft CRT (the one Clang is using) deprecates open() in favor of
> > _sopen_s() and issues a warning, and we're targeting -Werror. Disabling all
> > such warnings (_CRT_SECURE_NO_DEPRECATE) doesn't seem right when CRT vendor
> > encourages using alternatives. This is the primary reason for open()
> > wrappers in v6.
> >
>
> that seems a bit shortsighted to me. By creating wrappers that restrict
> functionality to the least common demoninator of supported platforms restricts
> what all platforms are capable of. For example, theres no reason that the eal
> library shouldn't be able to open a file O_TRUNC or O_SYNC just because its
> complex to do it on a single platform.
The purpose of these wrappers is to maximize reuse of common code. It doesn't
require POSIX par se, it's just implemented in terms of API that had been
available on all supported OSes until Windows target was introduced. Wrapper
interface is derived from common code requirements.
> The API should be written to support the full range of functionality on all
> platforms, and the individual implementations should write the code to make that
> happen, or return an error that its unsupported on this particular platform.
IMO, common code, by definition, should avoid partial support of anything.
> I'm not saying that you have to implement everything now, but you shouldn't
> restrict the API from being able to do so in the future. Otherwise, in the
> future, if someone wants to implement O_TRUNC support (just to site an example),
> they're going to have to make a change to the API above, and alter the
> implementation for all the platforms anyway. You may as well make the API
> robust enough to support that now.
I agree that these particular wrappers can have a lot more options, so
probably flags would be better. However, I wouldn't add parameters that
have partial support, namely, permissions. What do you think of the following
(names shortened)?
enum mode {
RO = 0, /* write-only is not portable */
RW = 1,
CREATE = 2 /* always 0600 equivalent */
};
eal_file_open(const char *path, int mode);
On Fri, Jun 05, 2020 at 03:16:03AM +0300, Dmitry Kozlyuk wrote:
> On Thu, 4 Jun 2020 17:07:07 -0400
> Neil Horman <nhorman@tuxdriver.com> wrote:
>
> > On Wed, Jun 03, 2020 at 03:34:03PM +0300, Dmitry Kozlyuk wrote:
> > > On Wed, 3 Jun 2020 08:07:59 -0400
> > > Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > [snip]
> > > > > +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.
> > >
> > > See 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
> > >
> > > There is a number of caveats, which IMO make this approach better:
> > >
> > > 1. Filesystem permissions on Windows are complicated. Supporting anything
> > > other than 0600 would add a lot of code, while EAL doesn't really need it.
> > > Microsoft's open() takes not permission bits, but a set of flags.
> > >
> > > 2. Restricted interface prevents EAL developers from accidentally using
> > > features not supported on all platforms via a seemingly rich API.
> > >
> > > 3. Microsoft CRT (the one Clang is using) deprecates open() in favor of
> > > _sopen_s() and issues a warning, and we're targeting -Werror. Disabling all
> > > such warnings (_CRT_SECURE_NO_DEPRECATE) doesn't seem right when CRT vendor
> > > encourages using alternatives. This is the primary reason for open()
> > > wrappers in v6.
> > >
> >
> > that seems a bit shortsighted to me. By creating wrappers that restrict
> > functionality to the least common demoninator of supported platforms restricts
> > what all platforms are capable of. For example, theres no reason that the eal
> > library shouldn't be able to open a file O_TRUNC or O_SYNC just because its
> > complex to do it on a single platform.
>
> The purpose of these wrappers is to maximize reuse of common code. It doesn't
> require POSIX par se, it's just implemented in terms of API that had been
> available on all supported OSes until Windows target was introduced. Wrapper
> interface is derived from common code requirements.
>
Sure, and I'm fine with that. What I'm concerned about is implementing wrappers
that define their APIs in terms of whats currently implemented. Theres no
reason that the existing in use feature set won't be built upon in the future,
and the API should be able to handle that.
> > The API should be written to support the full range of functionality on all
> > platforms, and the individual implementations should write the code to make that
> > happen, or return an error that its unsupported on this particular platform.
>
> IMO, common code, by definition, should avoid partial support of anything.
>
I disagree. Anytime you abstract an implementation to a more generic api, you
have the possibility that a given implementation won't offer full support for
all of the APIs features, and thats ok, as long as there are no users of the
features, and a given implmentation properly returns an error when their usage
is attempted. The expectation then is, that the user of the feature will add
the feature to all implementations, so that the code can remain portable. What
you shouldn't do is define the API such that those features can't be implemented
without having to change the API, as that runs the potential risk of having to
modify the ABI. Thats probably not the case here, but the notion stands. If
you write the API to encompass the superset of supported platforms features, the
rest is implementation details.
> > I'm not saying that you have to implement everything now, but you shouldn't
> > restrict the API from being able to do so in the future. Otherwise, in the
> > future, if someone wants to implement O_TRUNC support (just to site an example),
> > they're going to have to make a change to the API above, and alter the
> > implementation for all the platforms anyway. You may as well make the API
> > robust enough to support that now.
>
> I agree that these particular wrappers can have a lot more options, so
> probably flags would be better. However, I wouldn't add parameters that
> have partial support, namely, permissions.
But windows does offer file and folder permissions:
https://docs.microsoft.com/en-us/previous-versions/windows/it-pro/windows-2000-server/bb727008(v=technet.10)?redirectedfrom=MSDN
As you said, they are complicated, and the security model is different, but
those are details that can be worked out/emulated when the need arises.
> What do you think of the following
> (names shortened)?
>
> enum mode {
> RO = 0, /* write-only is not portable */
> RW = 1,
> CREATE = 2 /* always 0600 equivalent */
> };
>
> eal_file_open(const char *path, int mode);
>
Yeah, that makes sense to me. I'd be good with that.
Neil
> --
> Dmitry Kozlyuk
>
@@ -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>
@@ -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;
@@ -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_ */
@@ -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
@@ -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
@@ -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)
new file mode 100644
@@ -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;
+}
+
+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;
+}
+
+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;
+}
new file mode 100644
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2020 Dmitry Kozlyuk
+
+sources += files(
+ 'eal_file.c',
+)