Re: [Qemu-devel] [PATCH] Add compat eventfd header

[Alexander Graf]

On 29.06.2011, at 15:59, Michael S. Tsirkin wrote:

> On Wed, Jun 29, 2011 at 03:22:33PM +0200, Alexander Graf wrote:
>> 
>> On 29.06.2011, at 15:11, Michael S. Tsirkin wrote:
>> 
>>> On Wed, Jun 29, 2011 at 03:02:46PM +0200, Alexander Graf wrote:
>>>> 
>>>> On 28.06.2011, at 17:35, Michael S. Tsirkin wrote:
>>>> 
>>>>> Support build on RHEL 5.X where we have syscall for eventfd but not
>>>>> userspace wrapper.
>>>>> 
>>>>> (cherry-picked from commit 9e3269181e9bc56feb43bcd4e8ce0b82cd543e65
>>>>> in qemu-kvm.git).
>>>>> 
>>>>> Signed-off-by: Michael S. Tsirkin <address@hidden>
>>>>> ---
>>>>> compat/sys/eventfd.h |   13 +++++++++++++
>>>>> configure            |    4 +++
>>>>> 2 files changed, 16 insertions(+), 0 deletions(-)
>>>>> create mode 100644 compat/sys/eventfd.h
>>>>> 
>>>>> diff --git a/compat/sys/eventfd.h b/compat/sys/eventfd.h
>>>>> new file mode 100644
>>>>> index 0000000..f55d96a
>>>>> --- /dev/null
>>>>> +++ b/compat/sys/eventfd.h
>>>>> @@ -0,0 +1,13 @@
>>>>> +#ifndef _COMPAT_SYS_EVENTFD
>>>>> +#define _COMPAT_SYS_EVENTFD
>>>>> +
>>>>> +#include <unistd.h>
>>>>> +#include <syscall.h>
>>>>> +
>>>>> +
>>>>> +static inline int eventfd (int count, int flags)
>>>> 
>>>> coding style seems wrong.
>>> 
>>> What exactly? Two empty lines?
>> 
>> The space between d and ( I'd say. Just put it in checkpatch and verify it 
>> :).
> 
> Will fix.
> 
>>> 
>>>> However, I'm not sure I like the idea of adding this code in qemu. 
>>>> Wouldn't the RHEL5 libc be a better place for such a wrapper?
>>>> 
>>>> 
>>>> Alex
>>> 
>>> My guess (I don't speak for red hat here) is that's unlikely to be
>>> patched anytime soon.  It helps me when I need to use such a box,
>>> and the cost seems negligeable. What's the drawback?
>> 
>> Well, you need to make sure that it only gets included on Linux systems and 
>> if there's ever some more compatibility wrapping around the syscall 
>> (unlikely, but you never know), this could potentially break.
> 
> Nope, this gets included last (-idirafter) so if it breaks it's broken
> anyway.
> 
>> Also, who defines SYS_eventfd? What if you're trying to build this code on 
>> SLES10 for example, which does not have the syscall and thus doesn't have it 
>> defined? Would compilation simply break?
>> 
>> 
>> Alex
> 
> Here's what happens:
> 1. configure runs
> 2. configure tries to compile a test program
>       - if eventfd.h exists in system compat is ignored
>       - if eventfd.h does not exist in system compat is used
>       - if compat is used but does build program does not compile
>       - if program does not compile eventfd is disabled

Sure, but the cflags is added nevertheless, right? So you end up including a 
header file that uses undefined constants or even includes random header files 
that don't necessarily exist on your OS.

Or is sys/eventfd.h only #include'd when the config option is set? Then it 
should be safe. However, it might make sense to double-check that inside the 
header itself and #error out in case the config option is not set, so that this 
gets caught easily.


Alex