On Wed, Jun 29, 2011 at 05:30:24PM +0200, Alexander Graf wrote: > > On 29.06.2011, at 17:28, Michael S. Tsirkin wrote: > > > 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 <m...@redhat.com> > >>>>>>> --- > >>>>>>> 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. > > I wouldn't call it needed, but I'd usually say better safe than sorry :). > > > We should also check ifdef __linux__. > > Good point, yes :) > > > Alex
Will do, thanks for the review.
