[Bug 225784] Merge Review: gdbm

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=225784


Stepan Kasal <skasal at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|needinfo?(skasal at redhat.com |
                   |)                           |




--- Comment #12 from Stepan Kasal <skasal at redhat.com>  2009-04-17 06:23:58 EDT ---
Thank you for the review.
I have fixed most of the issues, the remaining ones are explained below.
Tell me if I missed any.

(In reply to comment #11)
> gdbm.x86_64: W: shared-lib-calls-exit /usr/lib64/libgdbm.so.2.0.0
> exit at GLIBC_2.2.5
> gdbm.x86_64: W: shared-lib-calls-exit /usr/lib64/libgdbm.so.2.0.0
> exit@@GLIBC_2.2.5
> 
> Should be investigated.

exit() is called from _gdbm_fatal() which gets called if a malloc or read
fails.

The reason why calling exit from a library is a bad idea is that the caller
should get a chance to clean up.  With gdbm, you can register a hook function
that gets called from _gdbm_fatal before the exit. This may not be the best
design decision, but it still somehow works.

Concerning this and the fact that the code has not changed the last five years,
I conclude that is would not be a good idea to try to patch this issue.
(At leat until a real life problem related to the issue appears.)

> * 1.8.3 was released but not sure whether is it good idea to incorporate it

Might be a good idea, but I believe that can be postponed after finishing this
review.

> Patch0: gdbm-1.8.0-jbj.patch
> Patch1: gdbm-1.8.0-fhs.patch
> Patch3: gdbm-1.8.0-64offset.patch
> 
> Could be: 0-1-2

I don't think it is advisable to renumber each time a patch is dropped.
Consequently, the sequence of the numbers can hardly be maintained.

> * discouraged: %makeinstall install-compat
> 
> https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

Quote: "%makeinstall macro [...] must NOT be used when make install
DESTDIR=%{buildroot} works"

grep -r DESTDIR gdbm-1.8.0 returns nothig.  Most projects get DESTDIR support
for free from Automake but this one does not use Automake, as mentioned above.

> > # refresh config.sub, the original one does not recognize "redhat"
> > # as vendorname:
> > for f in /usr/share/automake-1.1?/config.sub; do
> >   :
> > done
> > cp -p $f .
> > libtoolize --force --copy
> > aclocal
> > autoconf
> 
> Perhaps autoreconf and patching the build system seems better to me. But not
> that important.

config.{sub,guess} scripts are totally autonomous shell script.
They can get copied to the project by "automake --add-missing --force-missing" 
but, unfortunately, this project does not use Automake.

Otherwise, autoreconf is just another way to call the appropriate tools.
Calling them directly is not a bad idea either, it is more transparent.

Perhaps you meant that we could patch the build system to use Automake.
That's a valid idea, I might decide to do that in future.
But keeping the original build system should also be acceptible for the review.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.