On Thu, May 28, 2009 at 2:47 PM, Doug Goldstein <cardoe(a)gentoo.org> wrote:
On Thu, May 28, 2009 at 9:48 AM, Daniel P. Berrange
<berrange(a)redhat.com> wrote:
> On Thu, May 28, 2009 at 09:04:55AM -0500, Doug Goldstein wrote:
<snip>
>
> I don't much like this function with the mix of fixe length buffers,
> and fixed length malloc()'s. I think it'd be better to split out the
> code for breaking $PATH into a list of strings into a separate function,
> eg
>
> int virSplitPath(const char *src, const char **dst);
>
> So it'd take the $PATH value, and return a list of strings in 'dst', and
> the number of strins as the return value.
>
> Then the virFindFileInPath() method, would be better to iterate over
> this, and use virAsprintf() to build the full path, rather than
> relying on fixed size buffers.
>
The point of using the fixed length buffer is for memory
fragmentation. While I will agree that it may be confusing at first
glance, the point is that libvirtd is a long running daemon and in a
short term scope a little extra memory usage is acceptable than to
fragment the memory space slowly over time. Since the scope of the
first variable is just the scope of the function, a fixed length
buffer is simply just some room on the stack and its gone once the
function is out of scope. The return value is just allocated once and
returned.
Implementing the pure allocation method as described above would
result in an unnecessary amount of small allocations. One for the char
array. One per each piece of path (on Fedora 10 there are 10
components to a default install in $PATH). Then one for path piece +
binary name. This would result in 21 allocations per call. That's a
lot of memory churn for a short running piece of code.
--
Doug Goldstein
Here's v2 of the patch. Changed to use goto and VIR_ALLOC/FREE
--
Doug Goldstein