[Home]

Summary:ASTERISK-04549: [patch] remove libstrfunc dependency
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2005-07-10 16:25:13Date Closed:2008-01-15 15:41:28.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) strndup.071405.1038.patch
( 1) strndup.diff
( 2) strndup.strcasestr.patch.v2.txt
Description:Recently, one call to strndup() caused compilation to break on FreeBSD
where the function is not part of the string library.
It was fixed by adding a dependency on libstrfunc, but in my opinion
this is the wrong approach because the fewer dependencies we have,
the easier is to maintain the code, and in this particular case
as the function is trivial to rewrite and incorporate in the tree.
The attached patch provides an implementation of ast_strndup()
written from scratch which does what we need.

****** ADDITIONAL INFORMATION ******

feel free to modify the implementation of the function - if you
like a single return point in ast_strndup it is trivial to do so.
I believe it makes no sense to make the function inline because
all the cost is in the malloc() (and the associated locking),
and the only use we have of this function is upon receipt of a
message, so the savings would be hardly measurable.
Comments:By: Tilghman Lesher (tilghman) 2005-07-10 23:33:23

If you're going to do this, why not make ast_strndup() a macro for strndup() and a function for FreeBSD?  This would make more sense, as ast_ functions defined for common library routines differ in functionality, whereas this one is just another (unnecessary for Linux) implementation.

This is similar to the ast_strcasestr() already in utils.c.
By: Luigi Rizzo (rizzo) 2005-07-11 15:14:50

it is a portability issue. If a function is only available on a
fraction of the platforms you care about (basically it's only linux,
as far as i can tell), and it is trivial (the full code is 100 bytes)
you are better off reimplementing it with a private name than tracking
dependencies and polluting the makefiles and code with #ifdefs

If you care about performance and code size etc,
i suggest to look at the half dozen or more patches that i submitted in
the past 2..4 weeks and are sitting there mostly unreviewed.
They include massive simplifications in terms of complexity
(and code size) in core parts of asterisk.
By: Tilghman Lesher (tilghman) 2005-07-11 15:26:56

The bug marshals are here to ensure that necessary changes are made before the committers review the patches.  If you will not accept hints given by the bug marshals, your patches are likely to sit idle and eventually be closed due to 'lack of interest of the original poster'.
By: silik0n (silik0n) 2005-07-13 13:33:16

this patches fixes my OSX compilation problems
By: twisted (twisted) 2005-07-13 13:42:23

Corydon76, There is not a problem with creating an internal function to handle all *known* platforms that have an issue with a certain function.  I am placing this patch into code review.
By: Tilghman Lesher (tilghman) 2005-07-13 13:53:40

twisted:  I agree, but I think it should follow the implementation of ast_strcasestr().  Why should we have two different ways of doing this in CVS?
By: silik0n (silik0n) 2005-07-13 19:25:35

actually just using a single call on ALL platforms will clean up the code.

This patch needs to be commited sooner then later as it does resolve the issue.

the other way around using this code which would be a better fix would to use autoconf and have it check for strndup and ifndef the private function if its not needed but i dont see that happening anytime soon

also this is not really a 'General' thing but a Serious Portability issue
By: silik0n (silik0n) 2005-07-13 19:28:50

and also, this is not just a FreeBSD issue there are several platforms that do not have this call in their libc implementations. OSX is one of them
By: Tilghman Lesher (tilghman) 2005-07-13 20:48:51

Well, if rizzo won't do it, I will.  Patch uploaded that does strndup the same way that Asterisk already does strcasestr.
By: silik0n (silik0n) 2005-07-13 22:10:36

Unify the above 2 patches, and move strcasestr completely to ast_strcasestr.

the reason for doing this is to allow easier code reviewability by eliminating dependancy
By: Tilghman Lesher (tilghman) 2005-07-13 22:35:00

What are you talking about?  There aren't any dependencies already.

Unless you're going to rewrite libc and stick everything in Asterisk.
By: silik0n (silik0n) 2005-07-13 22:56:31

v2 uploaded to clean up a stupid mistake
By: silik0n (silik0n) 2005-07-13 23:03:01

as for how its fixed i could really care less as long as it gets fixed... there are 3 workable patches here someone please pick one
By: Michael Jerris (mikej) 2005-07-14 05:51:20

I vote for corydon's implementation.
By: silik0n (silik0n) 2005-07-14 08:08:44

Cordyon's works fine after modification strnlen is not a universal call tho

for (len=0; len<n; len++)
if (s[len] == '\0')
break;

does the same thing tho


By: Tilghman Lesher (tilghman) 2005-07-14 10:16:44

Okay, implementation done of ast_strnlen, too.
By: Michael Jerris (mikej) 2005-07-14 10:21:02

would this same way work well for strtoq and other functions that are not in some platforms (see strcompat.c).  Is there a good way to test if the function exsists globally without checking the specific platform and including if it is unavailable?
By: silik0n (silik0n) 2005-07-14 10:41:29

Added update to corydon's patch, just a couple of typo's in there

Otherwise looks great and works for me now

As for other calls and detecting the only good way is autoconf (and I'm not nessecarily a fan of that)
By: twisted (twisted) 2005-07-14 11:01:19

corydon76's patch fails right off the bat for including a file that doesn't exist.
By: Tilghman Lesher (tilghman) 2005-07-14 11:03:03

Detection of the _existence_ of a named routine is as simple as an #ifdef.  Knowing whether that routine conforms to a specific prototype is a bit more difficult.
By: twisted (twisted) 2005-07-14 11:03:11

Also, the patch from corydon76 has two different cast types, in *strings.h* (note, NOT string.h as included), it's cast as char *, whereas in the code, it's cast as size_t.  This fails compile.
By: Tilghman Lesher (tilghman) 2005-07-14 11:06:42

Ooops.  Let's try asterisk/strings.h
By: Kevin P. Fleming (kpfleming) 2005-07-14 20:32:21

Committed to CVS HEAD, with some minor mods, but as discussed on the conference call I'm going to work it over a bit tonight and make it autoconf-ready and also move the library-type functions into a lib subdirectory with a separate Makefile so that future portability problems can be handled a little more gracefully. Thanks to all who contributed on this one!
By: Digium Subversion (svnbot) 2008-01-15 15:41:27.000-0600

Repository: asterisk
Revision: 6133

U   trunk/Makefile
U   trunk/channel.c
U   trunk/include/asterisk/strings.h
U   trunk/utils.c

------------------------------------------------------------------------
r6133 | kpfleming | 2008-01-15 15:41:27 -0600 (Tue, 15 Jan 2008) | 2 lines

first phase of proper fix for portable string function problems (bug ASTERISK-4549)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6133
By: Digium Subversion (svnbot) 2008-01-15 15:41:28.000-0600

Repository: asterisk
Revision: 6134

U   trunk/utils.c

------------------------------------------------------------------------
r6134 | twisted | 2008-01-15 15:41:28 -0600 (Tue, 15 Jan 2008) | 3 lines

Fix breakage caused by bug ASTERISK-4549.  Next time let's test the build on
linux before commiting a portability patch ;)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6134