[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Add compat eventfd header
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] Add compat eventfd header |
Date: |
Wed, 29 Jun 2011 18:33:04 +0300 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <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.
>
> 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.
- [Qemu-devel] [PATCH] Add compat eventfd header, Michael S. Tsirkin, 2011/06/28
- Re: [Qemu-devel] [PATCH] Add compat eventfd header, Alexander Graf, 2011/06/29
- Re: [Qemu-devel] [PATCH] Add compat eventfd header, Michael S. Tsirkin, 2011/06/29
- Re: [Qemu-devel] [PATCH] Add compat eventfd header, Alexander Graf, 2011/06/29
- Re: [Qemu-devel] [PATCH] Add compat eventfd header, Michael S. Tsirkin, 2011/06/29
- Re: [Qemu-devel] [PATCH] Add compat eventfd header, Alexander Graf, 2011/06/29
- Re: [Qemu-devel] [PATCH] Add compat eventfd header, Michael S. Tsirkin, 2011/06/29
- Re: [Qemu-devel] [PATCH] Add compat eventfd header, Alexander Graf, 2011/06/29
- Re: [Qemu-devel] [PATCH] Add compat eventfd header,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH] Add compat eventfd header, Paolo Bonzini, 2011/06/29