[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: [patch] relayd - system/5716 update
From:       Cédric_Berger <cedric () berger ! to>
Date:       2008-02-29 11:29:14
Message-ID: 47C7EC8A.6080303 () berger ! to
[Download RAW message or body]

Nigel J. Taylor wrote:
> Thanks, for that point, I had considered it, but stuck with something
> closer to the original. There maybe byte ordering issues, and alignment,
> which need to be considered, before doing the compare as 32bit/16bit
> integers, you need to convert them first using ntohl, ntohs.

Huh?
Why would you need to convert using ntohl, ntohs to see if two integer
are equal?

  if (a == b) then (ntohs(a) == ntohs(b))

> You could
> avoid the conversion to integers provided there are no alignment issues,

Huh?
Which alignement issues?

Cedric

PS: and is the bcopy above really needed too? I doubt it...


> then the results will vary between different architectures. memcmp might
> be more efficient than doing the convert and integer compare.
> 
> 
> Regards
> 
> Nigel Taylor
> 
> 
> Cidric Berger wrote:
>> Nigel J. Taylor wrote:
>>> A small fix to relayd/relay.c follows below, the compare in relay_cmp_af
>>> added the difference between the ip address and ports together, in rare
>>> cases this could return zero when the ip address byte difference is
>>> exactly negated by the difference between port bytes.
>>>
>>> I have progressed slightly further on the 100% CPU usage. I know where
>>> the problem is SPLAY tree for the sessions, the SPLAY_INSERT,
>>> SPLAY_REMOVE's appear to end up with a corrupt tree where the tree loops
>>> back on itself, then the next call to either will just loop. A
>>> modification of relay.c to display SPLAY tree show that entries are not
>>> being correctly removed/inserted. I haven't worked out if the fault is
>>> within the SPLAY macros, or relayd use of SPLAY macros.
>>>
>>> Checking for other uses of SPLAY_xxxx only systrace and kern/subr_pool.c
>>> appear to be using the SPLAY_xxxx macros, within OpenBSD. If issue turns
>>> out to be within SPLAY_xxx macros, this gives an idea what else might be
>>> affected.
>>>
>>> I found a splay-test in regress, no errors reported from this. I am
>>> thinking about trying to modify this to use the same sequence of
>>> SPLAY_INSERT, SPLAY_REMOVE that causes the loop with relay.c.
>>>
>>> Regards
>>>
>>> Nigel Taylor
>>>
>>>
>>>
>>> Index: relay.c
>>> ===================================================================
>>> RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
>>> retrieving revision 1.84
>>> diff -u -r1.84 relay.c
>>> --- relay.c    13 Feb 2008 11:32:59 -0000    1.84
>>> +++ relay.c    28 Feb 2008 20:41:51 -0000
>>> @@ -2796,6 +2796,7 @@
>>>  int
>>>  relay_cmp_af(struct sockaddr_storage *a, struct sockaddr_storage *b)
>>>  {
>>> +    int ret = -1;
>>>      struct sockaddr_in ia, ib;
>>>      struct sockaddr_in6 ia6, ib6;
>>>
>>> @@ -2804,19 +2805,23 @@
>>>          bcopy(a, &ia, sizeof(struct sockaddr_in));
>>>          bcopy(b, &ib, sizeof(struct sockaddr_in));
>>>
>>> -        return (memcmp(&ia.sin_addr, &ib.sin_addr,
>>> -            sizeof(ia.sin_addr)) +
>>> -            memcmp(&ia.sin_port, &ib.sin_port,
>>> -            sizeof(ia.sin_port)));
>>> +        ret = memcmp(&ia.sin_addr, &ib.sin_addr,
>>> +            sizeof(ia.sin_addr));
>>> +        if (ret == 0)
>>> +            ret = memcmp(&ia.sin_port, &ib.sin_port,
>>> +                sizeof(ia.sin_port));
>> Is memcmp really the fastest way to compare 16bit and 32bit integers?
>>
>> Cedric

[prev in list] [next in list] [prev in thread] [next in thread]