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

[Michael S. Tsirkin]

On Wed, Jun 29, 2011 at 04:07:50PM +0200, Alexander Graf wrote:
> 
> 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?

Yes.

> Then it should be safe.

Good.

> 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

That makes the original testing that the header works a bit trickier:
we have to add -DCONFIG_EVENTFD. But I can do that if you think it's
needed. We should also check ifdef __linux__.


-- 
MST