Re: HEADSUP: BSD iconv coming to the base system with default off

On Thu, 24 Feb 2011 22:40:48 +0000
Gabor Kovesdan <gabor_at_kovesdan.org> wrote:

> Hi Folks,
> 
> I've got some time again to keep working on iconv. The last time I 
> posted a patch, there were problems with some ports but apart from
> this it proved to be quite mature, so I decided to commit it to the
> base system but the default setting will be disabled. It can be
> enabled by setting the WITH_ICONV knob but whoever does it will take
> into account that it may break some ports from being built. However,
> this change will help me with future work and will also help
> interested people in getting involved in the testing. The rather big
> changeset is coming soon.
> 
> Regards,
> Gabor Kovesdan

Hi Gabor,

for whatever historic reason I had WITH_ICONV in /etc/make.conf on my
desktop, so I got to be your guinea pig without conscious consent for
that role on my part. I did hit several issues so far:

1. mutt-devel port does not consider our implementation as 'good
enough'. It runs the test below, which returns 1, while GNU libiconv
returns 0.

| #include <iconv.h>
| int main()
| {
|   iconv_t cd;
|   char buf[4];
|   char *ob;
|   size_t obl;
|   ob = buf, obl = sizeof(buf);
|   return ((cd = iconv_open("UTF-8", "UTF-8")) != (iconv_t)(-1) &&
|           (iconv(cd, 0, 0, &ob, &obl) ||
|            !(ob == buf && obl == sizeof(buf)) ||
|            iconv_close(cd)));
| }

2. The implementation of internal locking in citrus_locking.h is a
strange one. Here is why:

Do you really want to have the lock declared as static in header file?
Even is so, please note that declaring data in header file is confusing
and having three locks with the same name is not making the library any
easier to debug.
 
You need to use proper XXX_STATIC_INITIALIZER constant to initialize
the lock statically, instead of declaring it as static. The code only
works on FreeBSD due to luck and will break if our typedefs for
pthreads lock primitives will ever grow to be the proper structures
instead of simple pointers.

The locking in citrus_mapper.c is broken in case of parallel and
sequential mappers. The file grabs the lock and calls _mapper_open. If
mapper being loaded happens to be of a composite type, it will in turn
try to invoke _mapper_open on subordinate mappers recursively and will
deadlock waiting for the lock it already owns.

And at last, by are you using rwlock if all locking you ever do is an
exclusive one?

3. There are dangerous vestiges of iconv.dir support in citrus_iconv.c
file, _citrus_iconv_open function. You call _lookup_alias there using
an uninitialized path variable, causing every iconv-using program to
try and access <garbage>.db file. While there, since you are not using
iconv.dir, would it make sense to just merge iconv_std and iconv_none
into libc? Same goes to various flavors of libmapper_*.so, is thee a
reason why each comes in its own trivial shared library?

4. Most of _citrus_<blah> functions are internal interface between
encoders and mappers and do nto constitute public ABI we as a project
are signing on to support forever. As such, they belong in private
namespace and should not pollute FBSD1.2.
 

--
Alexander Kabaev