Welcome! Log In Create A New Profile

Advanced

[PATCH] BUG/MEDIUM: pollers/kqueue: use incremented position in event list

Posted by PiBa-NL 
Hi Olivier,

Please take a look at attached patch. When adding 2 fd's the second
overwrote the first one.
Tagged it medium as haproxy just didn't work at all. (with kqueue.).
Though it could perhaps also be minor, as the commit has only been done
recently.?. Anyhow.. This seems to fix it :). Tried to go with a 'int
reference pointer' but couldn't find the best syntax to do it that way,
and this seems clean enough as well.. Though if you think a different
approach (thread local int  ?) or any other way is better please change
it or advice :)

Issue was introduced here:
http://git.haproxy.org/?p=haproxy.git;a=commit;h=6b96f7289c2f401deef4bdc6e20792360807dde4

Thanks,
PiBa-NL (Pieter)
From 3c60fdace2f23a8c6d070c8fafab660becb8c514 Mon Sep 17 00:00:00 2001
From: PiBa-NL <[email protected]>
Date: Thu, 10 May 2018 01:01:28 +0200
Subject: [PATCH] BUG/MEDIUM: pollers/kqueue: use incremented position in event
list

When composing the event list for subscribe to kqueue events, the index
where the new event is added must be after the previous events, as such
the changes counter should continue counting.

This caused haproxy to accept connections but not try read and process
the incoming data.

This patch is for 1.9 only
---
src/ev_kqueue.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/ev_kqueue.c b/src/ev_kqueue.c
index 926f77c..09e191c 100644
--- a/src/ev_kqueue.c
+++ b/src/ev_kqueue.c
@@ -33,10 +33,10 @@ static int kqueue_fd[MAX_THREADS]; // per-thread kqueue_fd
static THREAD_LOCAL struct kevent *kev = NULL;
static struct kevent *kev_out = NULL; // Trash buffer for kevent() to write the eventlist in

-static int _update_fd(int fd)
+static int _update_fd(int fd, int start)
{
int en;
- int changes = 0;
+ int changes = start;

en = fdtab[fd].state;

@@ -91,7 +91,7 @@ REGPRM2 static void _do_poll(struct poller *p, int exp)
activity[tid].poll_drop++;
continue;
}
- changes += _update_fd(fd);
+ changes = _update_fd(fd, changes);
}
/* Scan the global update list */
for (old_fd = fd = update_list.first; fd != -1; fd = fdtab[fd].update.next) {
@@ -109,7 +109,7 @@ REGPRM2 static void _do_poll(struct poller *p, int exp)
continue;
if (!fdtab[fd].owner)
continue;
- changes += _update_fd(fd);
+ changes = _update_fd(fd, changes);
}

if (changes) {
--
2.10.1.windows.1
Hi Pieter,

On Thu, May 10, 2018 at 01:12:40AM +0200, PiBa-NL wrote:
> Hi Olivier,
>
> Please take a look at attached patch. When adding 2 fd's the second
> overwrote the first one.
> Tagged it medium as haproxy just didn't work at all. (with kqueue.). Though
> it could perhaps also be minor, as the commit has only been done recently.?.
> Anyhow.. This seems to fix it :). Tried to go with a 'int reference pointer'
> but couldn't find the best syntax to do it that way, and this seems clean
> enough as well.. Though if you think a different approach (thread local int 
> ?) or any other way is better please change it or advice :)
>

Oops you're right, I'm an idiot, I think your patch is fine, Willy can you
please commit it ?

Thanks !

Olivier
Hi guys,

On Fri, May 11, 2018 at 01:57:10PM +0200, Olivier Houchard wrote:
> Hi Pieter,
>
> On Thu, May 10, 2018 at 01:12:40AM +0200, PiBa-NL wrote:
> > Hi Olivier,
> >
> > Please take a look at attached patch. When adding 2 fd's the second
> > overwrote the first one.
> > Tagged it medium as haproxy just didn't work at all. (with kqueue.). Though
> > it could perhaps also be minor, as the commit has only been done recently.?.
> > Anyhow.. This seems to fix it :). Tried to go with a 'int reference pointer'
> > but couldn't find the best syntax to do it that way, and this seems clean
> > enough as well.. Though if you think a different approach (thread local int 
> > ?) or any other way is better please change it or advice :)
> >
>
> Oops you're right, I'm an idiot,

Why did you think you where hired ? :-)

> I think your patch is fine, Willy can you please commit it ?

Sure, now done. Thanks!
Willy
On Fri, May 11, 2018 at 02:09:43PM +0200, Willy Tarreau wrote:
> Hi guys,
>
> On Fri, May 11, 2018 at 01:57:10PM +0200, Olivier Houchard wrote:
> > Hi Pieter,
> >
> > On Thu, May 10, 2018 at 01:12:40AM +0200, PiBa-NL wrote:
> > > Hi Olivier,
> > >
> > > Please take a look at attached patch. When adding 2 fd's the second
> > > overwrote the first one.
> > > Tagged it medium as haproxy just didn't work at all. (with kqueue.). Though
> > > it could perhaps also be minor, as the commit has only been done recently.?.
> > > Anyhow.. This seems to fix it :). Tried to go with a 'int reference pointer'
> > > but couldn't find the best syntax to do it that way, and this seems clean
> > > enough as well.. Though if you think a different approach (thread local int 
> > > ?) or any other way is better please change it or advice :)
> > >
> >
> > Oops you're right, I'm an idiot,
>
> Why did you think you where hired ? :-)
>

I thought it was because I was good looking, but that may actually prove your
point.

> > I think your patch is fine, Willy can you please commit it ?
>
> Sure, now done. Thanks!

Thanks !

Olivier
Sorry, only registered users may post in this forum.

Click here to login