Re: [PATCH] HttpHdrSc c++ refactoring: strnrchr

On 10/30/2011 12:55 PM, Kinkie wrote:
> On Fri, Oct 28, 2011 at 10:05 PM, Alex Rousskov wrote:

>> Can we just use memrchr() instead (and provide it if it is not available)?

> Now reverted to memrchr, relying on <cstring> - I'm not sure memrchr
> is part of ISO C.

According to a Linux man page, "The memrchr() function is a GNU
extension, available since glibc 2.1.91".

I think we should supply memrchr() when it is not available.

> I'll leave in compat/strnrchr.{h,cc} as it's done and shouldn't hurt.
> Please let me know if you want it removed.

It is up to you whether to add an unused function, but if you do add it,
please fix the description. To match the implementation, the description
should say that we scan from the beginning and stop at the end of
c-string or n-th character, whichever comes first.

I would also recommend checking whether your strnrchr() implementation
matches that of GNU API. Their documentation is even worse:
  http://gnugeneration.com/mirrors/kernel-api/r2320.html

>>> - temp = xstrndup (item, initiallen + 1);
>>> - if (!((target = strrchr (temp, ';')) && ...
>>> + if (!((target = strnrchr (item, initiallen+1, ';')) && ...

>> Why are we adding 1 in the above strnrchr() call? Does not the item have
>> no more than initiallen characters?

> Direct porting; in short, I don't know.

AFAICT, in the original code, xstrncpy() called from xstrndup() sets the
[initiallen] character to zero so it can never match ';'. The patched
code will check whether that character matches ';', which is wrong.
Consider something like "foo;;" as a test case, with item being "foo;"
and not "foo;;".

HTH,

Alex.
Received on Mon Oct 31 2011 - 18:56:57 MDT