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

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> 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)) {

Suggest

                  /*
                   * Check whether open() failed due to use of O_DIRECT,
                   * and set a more helpful error then.
                   */

>> > +            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.

Ah, got it now.

I'd prefer

                errno = EINVAL; /* restore first open()'s errno */
>> 
>> > +                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

You're right.  I misread the code.

Reviewed-by: Markus Armbruster <armbru@redhat.com>


Now back to my dislike of the #ifdeffery I voiced in reply to PATCH 2.

Code now:

    #ifdef O_CLOEXEC
        flags |= O_CLOEXEC;
    #endif /* O_CLOEXEC */

        ret = open(name, flags, mode);

    #ifndef O_CLOEXEC
        if (ret >= 0) {
            qemu_set_cloexec(ret);
        }
    #endif /* ! O_CLOEXEC */

        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);
                    [O_DIRECT not supported error...]
                    errno = EINVAL; /* close() clobbered earlier errno */
                    return -1;
                }
            }
    #endif /* O_DIRECT */
            [generic error...]
        }

Compare:

    #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 /* ! O_CLOEXEC */

        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);
                    [O_DIRECT not supported error...]
                    errno = EINVAL; /* close() clobbered earlier errno */
                    return -1;
                }
            }
    #endif /* O_DIRECT */
            [generic error...]
        }

I like this a bit better, but not enough to make a strong
recommendation, let alone demand.