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

[Daniel P . Berrangé]

On Wed, Aug 26, 2020 at 01:19:53PM +0200, Markus Armbruster wrote:

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

In v5 I've gone with neither approach, and instead spun off a helper
method qemu_open_cloexec which I think is clearer than both.

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 :|