Re: [PATCH v4 5/6] util: give a specific error message when O_DIRECT doesn't work

[Daniel P . Berrangé]

On Tue, Aug 25, 2020 at 05:19:53PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > A common error scenario is to tell QEMU to use O_DIRECT in combination
> > with a filesystem that doesn't support it. To aid users to diagnosing
> > their mistake we want to provide a clear error message when this happens.
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  util/osdep.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/util/osdep.c b/util/osdep.c
> > index a4956fbf6b..6c24985f7a 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -345,6 +345,19 @@ qemu_open_internal(const char *name, int flags, mode_t 
> > mode, Error **errp)
> >  
> >      if (ret == -1) {
> >          const char *action = flags & O_CREAT ? "create" : "open";
> > +#ifdef O_DIRECT
> > +        if (errno == EINVAL && (flags & O_DIRECT)) {
> > +            ret = open(name, flags & ~O_DIRECT, mode);
> > +            if (ret != -1) {
> > +                close(ret);
> > +                error_setg(errp, "Could not %s '%s' flags 0x%x: "
> > +                           "filesystem does not support O_DIRECT",
> > +                           action, name, flags);
> > +                errno = EINVAL; /* close() clobbered earlier errno */
> 
> More precise: close() may have clobbered

Either open or close in fact.

> 
> Sure open() can only fail with EINVAL here?

We don't care about the errno from the open() call seen in this context,
we're restoring the EINVAL from the previous open() call above this patch
contxt, that we match on with  "if (errno == EINVAL && ...)" line.

> 
> > +                return -1;
> > +            }
> > +        }
> > +#endif /* O_DIRECT */
> >          error_setg_errno(errp, errno, "Could not %s '%s' flags 0x%x",
> >                           action, name, flags);
> >      }
> 
> There is no qemu_set_cloexec().  Intentional?

We've called close() immediately after open(). Adding qemu_set_cloexec()
doesnt make it any less racy on platforms lacking O_CLOSEXEC

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|