xserver: Cleaning up memory allocation functions and macros

[Magnus Vigerlöf]

On lördag 28 april 2007, Daniel Stone wrote:
[...]
> > * Neither have I seen MEMBUG being defined anywhere. Will anyone miss
> > this feature if I remove it? There are code that ignore the case where
> > the memory allocation functions return NULL (the option functions
> > (addNewOption2 [1]), I don't know how to make it better without breaking
> > the interface though), so my gut feeling is that it's not used very
> > much/at all today.
>
> Not really.  I guess the best way of testing this would be to get a libc
> modification in which did the same.

I've used the memory allocation tracing features in glibc when tracking down 
the memory leaks in the hotplug path, maybe the hook-concept included in 
glibc can be used for this purpose when something like this is needed? Not 
usable for those system based on something else than glibc, I know..

> > * Is there a reason for the existence of both xmalloc (macro) and Xmalloc
> > (wrapper function)? (apply this question on all memory functions) I'll go
> > for the libc-variants wherever I can, but those functions that depends on
> > a slightly different semantic than the libc ones, which name standard
> > should be used? (I prefer the lower case prefix [xstrdup, xnf...].)
>
> Probably to do with ABI concerns from 1993.

Ok. So stick with the current and phase all these calls out as we go then. 
Good.

> > * Use standard libc-functions
> >   - For every call to alloc/calloc/realloc/... there *must* be a check if
> > it succeded to allocate the memory.
> >   - free() can take NULL as argument, so it's not needed to test for NULL
> > before calling it.
>
> Huzzah!  Thankyou.

:)

> > * xstrdup() will be defined as a macro which can handle being passed
> > NULL. strdup doesn't.
>
> Sounds sensible, but you could also flag a warning if --enable-debug is
> enabled, and we could fix all these cases and gradually transition to
> using normal strdup()?

Hmmm... If this is as easy as enabling a #warning if DEBUG is set, I think I 
need to make this a wrapper function instead. Otoh, that would make it a 
candidate to mark it with _X_DEPRECATED. I think I'll go for that instead if 
there's no objection.

> > * xfreeZ() will be defined as a macro that set the pointer to NULL after
> > freeing the memory with free(). Use *only* if the pointer must be set to
> > NULL for some reason.
>
> Is there any reason to do this?  It just encourages the x*() wrappers
> and makes the code less obvious to the casual observer.

The code in some places will look nicer. However, I'm not sure I want this 
either (I just had to toss the idea up and see if anyone would shoot it 
down). I'll remove this one. Thanks.

> > * xnf* will be implemented as wrapper functions that will call FatalError
> > if the libc-functions returned NULL when they shouldn't (i.e. more or
> > less kept as-is). NOTE: Avoid these, use proper error-propagation instead
> > (calling these should be considered a BUG).
>
> Indeed.  You can just mark these as _X_DEPRECATED and the compiler will
> complain every time.

And it's not that many really (200-400 places, thought it would be more..). 
But the tricky part will be to refactor the code so it'll handle the 
situation instead without breaking the interface. I'll change the call to one 
of these if proper checking is not made (and I can't fix it), so there will 
be some more, but we'll know where they are at least.

Thanks
  Magnus