Re: [Qemu-devel] [PATCH for-1.2] osdep: Fix compilation failure on BSD systems

[Peter Maydell]

On 16 August 2012 13:27, Stefan Weil <address@hidden> wrote:
> Am 16.08.2012 13:15, schrieb Peter Maydell:
>
>> Fix compilation failure on BSD systems (which don't have
>> O_DIRECT or O_NOATIME:
>> osdep.c:116: error: ‘O_DIRECT’ undeclared (first use in this function)
>> osdep.c:116: error: (Each undeclared identifier is reported only once
>> osdep.c:116: error: for each function it appears in.)
>> osdep.c:116: error: ‘O_NOATIME’ undeclared (first use in this function)
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>> PS: Do we care about O_DSYNC, O_RSYNC, O_SYNC? POSIX says those can be
>> used via fcntl() too...
>>
>>   osdep.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/osdep.c b/osdep.c
>> index 5b78cee..3b25297 100644
>> --- a/osdep.c
>> +++ b/osdep.c
>> @@ -113,7 +113,13 @@ static int qemu_dup_flags(int fd, int flags)
>>       }
>>         /* Set/unset flags that we can with fcntl */
>> -    setfl_flags = O_APPEND | O_ASYNC | O_DIRECT | O_NOATIME | O_NONBLOCK;
>> +    setfl_flags = O_APPEND | O_ASYNC | O_NONBLOCK;
>> +#ifdef O_NOATIME
>> +    setfl_flags |= O_NOATIME;
>> +#endif
>> +#ifdef O_DIRECT
>> +    setfl_flags |= O_DIRECT;
>> +#endif
>>       dup_flags &= ~setfl_flags;
>>       dup_flags |= (flags & setfl_flags);
>>       if (fcntl(ret, F_SETFL, dup_flags) == -1) {
>
>
> Would O_DSYNC be a good replacement here for an
> undefined O_DIRECT? block/raw-posix.c does it like that.

I think it would be better to have this function handle
O_DSYNC as a flag to go in setfl_flags independently of
whether O_DIRECT is defined or not; at this level I don't
think it makes sense to try to #define O_DIRECT O_DSYNC
(we are interpreting flags given us by a caller, not picking
flags to try to get the behaviour we want from a function
we're going to call).

> What about defining O_NOATIME and O_DIRECT in qemu-common.h
> when needed (like it is done for O_BINARY)?

Again, I think that would be a fix at the wrong level: it
might be OK if we wanted to allow people to call this function
with those flags even if the host OS didn't know about them,
but it's not what you need to make the function itself
perform correctly. What we might want is to add
#ifdef O_DSYNC
    setfl_flags |= O_DSYNC;
#endif
etc, but I'm not sure about that.

> But I don't want to delay 1.2, therefore
>
> Reviewed-by: Stefan Weil <address@hidden>

Thanks; this is indeed really just a minimal "make it compile"
fix.

-- PMM