| Message ID | 20200702125612.2176194-2-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: improve error reporting for unsupported O_DIRECT | expand |
On 7/2/20 2:56 PM, Daniel P. Berrangé wrote: > Currently we suggest that a filesystem may not support O_DIRECT after > seeing an EINVAL. Other things can cause EINVAL though, so it is better > to do an explicit check, and then report a definitive error message. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > util/osdep.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/util/osdep.c b/util/osdep.c > index 4829c07ff6..e2b7507ee2 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -332,9 +332,11 @@ int qemu_open(const char *name, int flags, ...) > } > > #ifdef O_CLOEXEC > - ret = open(name, flags | O_CLOEXEC, mode); > -#else > + flags |= O_CLOEXEC; > +#endif > ret = open(name, flags, mode); > + > +#ifndef O_CLOEXEC > if (ret >= 0) { > qemu_set_cloexec(ret); > } > @@ -342,8 +344,13 @@ int qemu_open(const char *name, int flags, ...) Too bad the git-diff removed 2 lines of context to add a comment instead. In case it helps other reviewers, what was stripped out of the context is this single line: #endif Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > #ifdef O_DIRECT > if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) { > - error_report("file system may not support O_DIRECT"); > - errno = EINVAL; /* in case it was clobbered */ > + int newflags = flags & ~O_DIRECT; > + ret = open(name, newflags, mode); > + if (ret != -1) { > + close(ret); > + error_report("file system does not support O_DIRECT"); > + errno = EINVAL; > + } > } > #endif /* O_DIRECT */ > >
Daniel P. Berrangé <berrange@redhat.com> writes: > Currently we suggest that a filesystem may not support O_DIRECT after > seeing an EINVAL. Other things can cause EINVAL though, so it is better > to do an explicit check, and then report a definitive error message. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > util/osdep.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/util/osdep.c b/util/osdep.c > index 4829c07ff6..e2b7507ee2 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -332,9 +332,11 @@ int qemu_open(const char *name, int flags, ...) > } > > #ifdef O_CLOEXEC > - ret = open(name, flags | O_CLOEXEC, mode); > -#else > + flags |= O_CLOEXEC; > +#endif > ret = open(name, flags, mode); > + > +#ifndef O_CLOEXEC > if (ret >= 0) { > qemu_set_cloexec(ret); > } I'd prefer something like #ifdef O_CLOEXEC flags |= O_CLOEXEC; ret = open(name, flags, mode); #else ret = open(name, flags, mode); if (ret >= 0) { qemu_set_cloexec(ret); } #endif Continues to duplicate open(), but spares me the effort to fuse two #ifdef sections in my head to understand what is being done in each case. > @@ -342,8 +344,13 @@ int qemu_open(const char *name, int flags, ...) > > #ifdef O_DIRECT > if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) { > - error_report("file system may not support O_DIRECT"); > - errno = EINVAL; /* in case it was clobbered */ > + int newflags = flags & ~O_DIRECT; > + ret = open(name, newflags, mode); I'd prefer the more concise ret = open(name, flags & ~O_DIRECT, mode); > + if (ret != -1) { > + close(ret); > + error_report("file system does not support O_DIRECT"); > + errno = EINVAL; > + } > } > #endif /* O_DIRECT */ The function now reports to stderr in just one of many failure modes. That's wrong. Looks like the next patch fixes this defect. I'd swap the two.
diff --git a/util/osdep.c b/util/osdep.c index 4829c07ff6..e2b7507ee2 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -332,9 +332,11 @@ int qemu_open(const char *name, int flags, ...) } #ifdef O_CLOEXEC - ret = open(name, flags | O_CLOEXEC, mode); -#else + flags |= O_CLOEXEC; +#endif ret = open(name, flags, mode); + +#ifndef O_CLOEXEC if (ret >= 0) { qemu_set_cloexec(ret); } @@ -342,8 +344,13 @@ int qemu_open(const char *name, int flags, ...) #ifdef O_DIRECT if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) { - error_report("file system may not support O_DIRECT"); - errno = EINVAL; /* in case it was clobbered */ + int newflags = flags & ~O_DIRECT; + ret = open(name, newflags, mode); + if (ret != -1) { + close(ret); + error_report("file system does not support O_DIRECT"); + errno = EINVAL; + } } #endif /* O_DIRECT */
Currently we suggest that a filesystem may not support O_DIRECT after seeing an EINVAL. Other things can cause EINVAL though, so it is better to do an explicit check, and then report a definitive error message. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- util/osdep.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)