dynamic providers and garbage collection

Jim Meyering jim at meyering.net
Mon Jan 17 20:10:55 UTC 2011


Jeff Darcy wrote:
> On 01/14/2011 04:49 PM, Jim Meyering wrote:
>> I have 3 tasks on my short-term TODO list:
>>  - mutex-protect the two provider iteration loops in replica.c
>>  - plug a small leak in meta.cpp
>>  - fix meta.cpp so it doesn't come up with a reference to malloc
>>      (as opposed to GC_malloc).  I think it does this via some mongodb
>>      method that uses a json-related inline extern-C function.
>
> This is related to a concern I had when looking at the diffs.  I see
> that free_ms has been eliminated in favor of GC voodoo, but the call to
> meta_query_stop might actually have been necessary.  It's probably also

Good catch.
I'll look into that.
It looks like the same applies to meta_attr_stop,
both of which (I now see) are unused functions.
I'd better start using tools/options (coverity, maybe clang, gcc lto)
that would catch that.

> worth mentioning that we need to be careful about leaks other than
> memory - e.g. file descriptors - with the Mongo stuff, and that

Definitely.  I regularly run valgrind with --track-fds=yes.
Once regular memory leaks are all dealt with, I start
looking at leaked file descriptors.

> switching to the C driver instead of C++ might make all of that easier.

Switching to mongodb's C driver sounds like a worthwhile investment.
That would eliminate iwhd's dependency on C++.

> Other comments, starting with the build process which I had to endure to
> check out these changes.
>
> (1) Is it really necessary or acceptable to do our own private git clone
> and build of gnulib,

That is the recommended practice.  There is currently no "libgnulib",
by design, though a compromise of sorts called libposix is in the works.
There are numerous benefits to making gnulib a submodule of projects
that use it.  Very few of the projects that use gnulib[*] do otherwise.
Note that the cost of the initial clone is typically a one-time deal.
I usually check out gnulib in a sibling directory, and then
use bootstrap's --gnulib-srcdir=../gnulib option to make it use that.

[*] Projects that use gnulib:
  http://git.sv.gnu.org/cgit/gnulib.git/tree/users.txt

> even running gnulib tests as part of our own "make
> check"?  This is an approach I had considered for dealing with CloudFS's

I'm 60:40 for retaining the code that arranges for "make check"
to run gnulib's tests of the small subset of its modules that we
actually use.  As it is, one test took more than a few seconds
so I told bootstrap to omit it.  For a project like iwhd that
has pretty significant prerequisites, it may not be as useful as
for those that must build/run on unusual or hard-to-port-to targets,
so I wouldn't mind terribly if folks want to drop it.

> dependency on GlusterFS, but rejected as generally too grotty and
> specifically unlikely to get by the Fedora package-review process.  If
> this is really the only way to make GC work, then shouldn't we do it

Not at all.  The fact that I tweaked gnulib's hash.c
sources merely indicated that I had not come up the GC learning
curve enough to use GC_register_finalizer as I did for libmicrohttpd.
I'll undo the hash.c kludge before pushing.

> also for other libraries (e.g. libmicrohttpd) on which we depend and
> that also allocate/free memory?

The gating factor is when we store pointers to memory
malloc'd by "other library" in a GC_malloc'd buffer.
In that case, we have to use GC_register_finalizer to
tell GC what's required to release its resources.

> (2) Bootstrap will fail if either gc/gc-devel are not installed, but
> neither bootstrap itself nor README-prereq mention them.

I realized that long ago, but never added the configure-time checks.
Will do.

> (3) There's a noted dependency on makeinfo, but no indication that it
> actually comes from the texinfo package.

When in doubt, you can run this:

    yum whatprovides /usr/bin/makeinfo

or perhaps more simply, run this to install it without
bothering to name its package:

    yum install /usr/bin/makeinfo

> (4) Autopoint is noted as a dependency but doesn't actually seem to be
> required (I built fine without it).  Just as well, since my F15 install
> of gettext (where it comes from on my other systems) doesn't seem to
> include it.

That is indeed an unnecessary dependency, for now.
Soon we'll have to enable gettext, so iwhd can print
translated diagnostics.  Then it will be required.

> OK, so much for the build.  Some concerns about the code too...
>
> (1) hash_initialize(13,...)?  Really?  Even ignoring the execrable state
> of gnulib documentation, that's a bit more obfuscated than necessary.

Yeah, it's better not to hard-code constants like that.
In this case 13 is simply a small prime initial table size.
I'll fix those, along with some similar preexisting ones:

    $ git grep -E '[0-9]{4},' rest.c
    rest.c:         MHD_SIZE_UNKNOWN, 65536, proxy_get_cons, pp, child_closer);
    rest.c:         ms->post = MHD_create_post_processor(conn,4096,
    rest.c:                 65536, proxy_query_func, ms, simple_closer);
    rest.c:         65536, proxy_query_func, ms, simple_closer);
    rest.c:         65536, root_blob_generator, ms, simple_closer);
    rest.c:         ms->post = MHD_create_post_processor(conn,4096,
    rest.c:         ms->post = MHD_create_post_processor(conn,4096,
    rest.c:         65536, parts_callback, ms, simple_closer);
    rest.c:         ms->post = MHD_create_post_processor(conn,4096,
    rest.c:         65536, prov_list_generator, ms, simple_closer);
    rest.c:         ms->post = MHD_create_post_processor(conn,4096,
    rest.c:         MHD_OPTION_CONNECTION_MEMORY_LIMIT, (size_t)1048576,

> (2) Is there really no decent strings-as-keys hash table in gnulib?
> I've heard the reasons for abandoning the GLib versions, but if we're
> going to change all that code anyway wouldn't it make more sense to use
> something that actually does that we want (e.g.
> http://judy.sourceforge.net/doc/JudySL_3x.htm) instead of NIH'ing
> something on top of gnulib?

IMHO, adding the tiny, tailored kv_* shim functions does not qualify as NIH.
I'll take a look at users of gnulib's hash module (thought not right away).
If more than one uses something similar to these kv_* functions
it might make sense to add it to gnulib.

> (3) What was the rationale for having prov_fmt/tmpl_prov_entry work
> differently than the other template functions, foregoing tmpl_ctx in

As mentioned in the log, the existing code would have silently
generated syntactically invalid output for any template or list
that ended up exceeding the hard-coded maximum buffer length.
There are several other cases of silent truncation that
I haven't addressed yet.

> favor of passing more arguments?  This is not merely aesthetic; JSON
> output is broken (spurious comma after the left square bracket) because
> z_offset isn't used and nothing was implemented to take its place.

Good catch.  I'll fix that.

> (4) The provider test should check for JSON as well as XML output.

Definitely.
Thanks for the review and all the feedback!


More information about the iwhd-devel mailing list