Re: [Qemu-devel] [PATCH 2/2] osdep: warn if open(O_DIRECT) on fails with EINVAL

[Eric Blake]

On 08/22/2013 03:29 AM, Stefan Hajnoczi wrote:
> Print a warning when opening a file O_DIRECT fails with EINVAL.  This
> saves users a lot of time trying to figure out the EINVAL error, which
> is typical when attempting to open a file O_DIRECT on Linux tmpfs.
> 
> Reported-by: Deepak C Shetty <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  util/osdep.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/util/osdep.c b/util/osdep.c
> index 685c8ae..62072b4 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -207,6 +207,13 @@ int qemu_open(const char *name, int flags, ...)
>      }
>  #endif
>  
> +#ifdef O_DIRECT

On some other projects I've worked with (hello gnulib!), the
compatibility headers do:

#define O_DIRECT 0

so that the rest of the code can just blindly use open(...,|O_DIRECT)
(provided, of course, that not having O_DIRECT semantics is not
fatal...).  If that is done, then this #ifdef will always be true...

> +    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {

...but the '(flags & O_DIRECT)' subclause would be false, so that's safe.

If O_DIRECT were always defined, then '#if O_DIRECT', or better yet, 'if
(O_DIRECT)', is a better way to conditionalize code that depends on it
being a useful value (no false positives when defined to 0, and by
moving the check from preprocessor to C, you get benefits of compiler
verification of type safety to avoid bitrot in what is otherwise dead code).

Grepping through qemu, I see that block/raw-posix.c has some weird
#ifdef'ery:

/* OS X does not have O_DSYNC */
#ifndef O_DSYNC
#ifdef O_SYNC
#define O_DSYNC O_SYNC
#elif defined(O_FSYNC)
#define O_DSYNC O_FSYNC
#endif
#endif

/* Approximate O_DIRECT with O_DSYNC if O_DIRECT isn't available */
#ifndef O_DIRECT
#define O_DIRECT O_DSYNC
#endif

which strikes me as odd (O_DSYNC and O_DIRECT have orthogonal, and
somewhat opposing goals - the former says don't return from write()
until data is on disk, which slows things down for safety; the latter
says "I'm a special user space, get out of my way, by not caching
anything, and leaving the flushing to me", to speed things up if the
user knows what they are doing - then again, the Linux man page hints
that they are often used together when worrying about guarantees for
synchronous I/O).

But enough of that side diversion - that one #define of O_DIRECT is not
related to the file you are touching.  I like the elegance of your patch
(nicer than the race in my double-open attempt).

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature