On Fri, Apr 1, 2011 at 5:49 AM, Andrew Beekhof <andrew(a)beekhof.net> wrote:
On Fri, Apr 1, 2011 at 12:44 PM, Daniel P. Berrange
<berrange(a)redhat.com> wrote:
> On Fri, Apr 01, 2011 at 12:37:49PM +0200, Andrew Beekhof wrote:
>> On Fri, Apr 1, 2011 at 12:32 PM, Daniel P. Berrange <berrange(a)redhat.com>
wrote:
>> > On Sat, Mar 26, 2011 at 02:37:07PM -0500, Russell Bryant wrote:
>> >> 1) Try to make the formatting more consistent. I went with what seemed
>> >> most common in the file (4 space indent, etc.). It would be good to
>> >> decide on a style guide, though, if that has not already been done.
>> >> Also trim trailing whitespace in passing.
>> >>
>> >> 2) Add checking for failures of memory allocations and handle them
>> >> gracefully.
>> >>
>> >> 3) Use calloc() instead of malloc() + memset().
>> >>
>> >> 4) Use asprintf() instead of a malloc() + snprintf().
>> >
>> > Matahari is already using a reasonable amount of GLib code,
>> > which will all abort-on-OOM, so adding OOM handling in these
>> > few places doesn't give you any real benefit & complicates
>> > the code with cleanup paths.
>> >
>> > It could be simpler to just convert all the malloc() usage into
>> > g_new/g_new0 which abort-on-OOM, and use g_strdup_printf() instaed
>> > of asprintf/snprintf, strdup -> g_strdup, etc, etc
>>
>> A step too far IMHO.
>> I'm not a fan of the glib replacements for malloc since you can't use
>> the "normal" free and you end up doing your head in trying to keep
>> track of which variables used which allocator.
>
> AFAICT, that is already a significant problem with matahari code, due to
> the very selective/partial use of GLib APIs.
It shouldn't be.
Essentially the rule is that only glib types (such as hashtables)
should be using glib allocator. Everything else should be using the
basic allocator (malloc/free).
If somewhere is not doing this then we need to fix it.
> If it made full use of all
> available GLib APIs, it would end up being clearer.
>
> Even if you don't want to use glib malloc replacements, it would make
> sense to define some thing matahari specific wrappers around the regular
> stdc malloc which would abort-on-OOM.
Nod, I've done this on other projects. Repeating here would be my
first inclination.
FWIW, a full migration to exclusively using glib APIs for this would
be my preference I think. If we were to add our own wrappers, we're
just re-implementing the glib wrappers. You're still stuck with the
complication of having to know when to use one or the other. I think
using only one set of allocation functions is a big win for
maintainability.
> There's really no point trying to
> handle OOM, once you've started using GLib for any non-trivial bit of
> code.
agree
I'm sold. I'll strip that stuff from the patches I have up so far. I
can work on a conversion to whatever is agreed upon.
--
Russell Bryant