[Rpm-maint] [PATCH 3/5] Optimize expandMacro function

[Panu Matilainen]

On 02/05/2013 06:31 AM, Alexey Tourbin wrote:
> From: Alexey Tourbin <at at altlinux.ru>
>
> This change brings the following optimizations:
> - In the beginning of the routine, the input string needs to be copied
>    only in non-terminated case.

This particular (and yes, obvious-looking) optimization was already 
reverted once because it breaks things: 
http://lists.rpm.org/pipermail/rpm-maint/2011-May/003026.html

So unless something else has changed in a way that avoids that breakage 
(which I doubt), NAK. It'd deserve a test-case though, and perhaps a 
comment in the code as well.

> - When mb->buf is NULL, expensive xcalloc call is replaced with xmalloc.
>    It suffices to ensure that mb->buf is (initially) null-terminated at
>    mb->tpos, and that mbAppend* routines follow this rule.
> - The inner per-character loop is replaced with call to strchr(s, '%').
>    Also note that the "trailing %" case is no longer handled by this part
>    of the code (this is now the case of no valid macro name).
> - New routine mbAppendMem now complements mbAppendStr.  Also, mbGrow
>    routine was factored to improve possibilities for inlining.

NAK, something here causes severe breakage in rpmbuild (a pile of 
segfault failures in the test-suite, and its not the above case of 
self-undefining macro as that's not tested currently). For example:

[pmatilai at localhost rpm]$ ./rpmbuild -ts 
tests/data/SOURCES/hello-2.0.tar.gz
*** glibc detected *** /home/pmatilai/repos/rpm/.libs/lt-rpmbuild: 
free(): invalid next size (fast): 0x00000000006c4620 ***
[...]
Aborted (core dumped)
[pmatilai at localhost rpm]$

When submitting patches, please make sure "make check" completes without 
errors. The test-suite is somewhat picky of how rpm is ./configure'd so 
you'll want to get it working with a known-good starting point (git 
master is expected to pass with zero errors at all times, or somebody 
has screwed up) before hacking further. It's a bit of a PITA as it takes 
so long to complete, but its also been a life-saver on numerous occasions...

Also please split the patch to one patch per independent change. The 
patch here isn't exactly big in terms of line-count but there's at least 
one or two different problems in it. With strict patch-per-change 
approach it's just SO much easier to review patches + spot the exact 
changes causing the problem(s). Not to mention when it comes to chasing 
down some previously unnoticed regression in an obscure corner case a 
year from now.

	- Panu -