Hi, I didn't write this code but I know the guy who did. I'll forward these comments.
On Fri, 09 Jul 2004 23:21:17 +1000, Peter Moulder wrote:
- The code for enable-binreloc=auto (which was previously the default) checked the build environment rather than the execution environment.
That's intentional. The idea is to entirely disable it when building binaries for a platform without /proc. The implicit assumption is that the presence of /proc is linked to the resultant binaries - ie all Linux binaries can assume its presence where as MacOS or Windows binaries will not and the code is therefore entirely disabled.
I can't remember what we do if /proc is not mounted at execution time but it was at compile time. I think we just assume it will be mounted, on the grounds that a ton of stuff (on Linux) breaks if /proc isn't present.
autopackage code prevents running inkscape from the build directory. (This problem is reduced if the source directory and the build directory are the same.)
It should fall back to using the configure-supplied PREFIX value.
OK, this makes sense. If the path determined automatically doesn't exist, it can fall back.
Also, there are a few problems with prefix.cpp code that make me wonder what other problems the autopackage code has:
- br_locate uses sscanf("%x-%x") and unsigned ints whereas
linux/fs/proc/array.c uses printf("%08lx-%08lx") and unsigned longs. This presumably leads to bugs on 64-bit platforms.
We did look into this, but neither of us knew enough about 64 bit systems to fix it. Can we just change sscanf to use %08lx-%08lx and unsigned longs as well?
Oh, looking at it, the code in our CVS is using sscanf("%lx-%lx") - I guess you have an out of date version. I'll look into updating it this weekend, hopefully. Maybe that'll fix some of the other issues.
- br_prepend_prefix needlessly requires that its path argument bewritable.
The version in our CVS does not appear to assume path is writable. Perhaps this was a bug that was fixed.
- br_strcat uses calloc, only to write over the space. It ineffect calculates strlen(str1) 3 times and strlen(str2) twice.
Right, I'm not sure why it uses calloc instead of malloc. Simple to fix though.
For the duplicate calculations, I guess it's worth noting that this code was written to be easily read and understood rather than high performance. Yes it's easy to fix. I'll run the patch by Hongli later.
- br_strndup has buffer overflow if size excedes the length of strby more than one.
I don't know why len is being used here rather than size.
- br_extract_prefix doesn't behave as its documentation indicates. Also it does an unnecessary strndup when it could use memrchr.
memrchr is a GNU extension so can't be used here.
I think there is just a thinko in the documentation. /usr/local/libfoo should read /usr/local/lib/libfoo.