talloc: Other minor issues/queries
jra at samba.org
Wed Oct 21 18:37:01 UTC 2020
On Wed, Oct 21, 2020 at 01:31:39PM -0500, Arran Cudbard-Bell wrote:
> Sorry yes, I should have been more specific. It clears the the existing
> name, and sets the name to be the program text.
> There's a number of places where this happens. All the *_append_*
> functions drop the name of the chunk and set it it to be the string
> For buffers its less relevant, but for structures... Well it just seems
> like a rather unfriendly thing for a type validation system to do.
> It seems like talloc_realloc* was originally envisioned primarily as a way
> of extending arrays or buffers, and not working with flexible array
> members in structs, or struct "compositing"
> The cost to fix this would appear to be two bits in the flags field.
> One to record whether the name was explicitly specified, one to
> record whether the name was originally const or not.
> There'd actually be some efficiency benefits to recording const vs not
> const in talloc_free_children. talloc_free_children currently walks
> over the list of children searching for a chunk that provides the name
> for the context.
> If we knew the context had a const name, then we could avoid that
> additional O(n) operation in the majority of cases.
> I'm happy to put together patches... Recording const vs non-const
> wouldn't seem to be that contentious. Preserving explicitly set names/
> types seems like it'd need more discussion.
That seems reasonable to me, but I'd rather Metze also
comment on this. It is a behavior change.
> > Second, I'd like to remove the memlimit code. Can you be
> > *really* clear as to what you are using them for, and
> > can we find some other way to do this ?
> We want to ensure heap allocations can't occur when a pool is full.
> a. In cases where these pools are composed of mprotected pages,
> or pages that will be mprotected.
> b. Where we want to trigger allocation of an additional "slab" of memory
> when an existing pool is used up. This is used where we're performing
> configuration parsing on startup.
> That's it, that's really our only use cases for memlimit. We never use
> them on any other type of chunk.
> A flag saying "don't allow heap allocations" would be absolutely fine
> for us in all cases, and much simpler to implement.
OK - that sounds *much* easier. An additional 'uint32_t flags'
argument passed into talloc_pool() and talloc_pooled_object()
with a 'CONFINE_TO_POOL' flags being the only one defined
at present would be the way to go IMHO.
This would have to be a new major rev of the library
as we'd be removing the memlimit code as well in order
to make the internals of talloc understandable again :-).
More information about the samba-technical