On Wed, Jun 03, 2020 at 03:34:03PM +0300, Dmitry Kozlyuk wrote:
> On Wed, 3 Jun 2020 08:07:59 -0400
> Neil Horman <nhor...@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
>
