Add sk_TYPE_new_reserve() function #4559

InfoHunter · 2017-10-19T17:50:31Z

This is a combination of sk_new_null and sk_reserve, to make it more
convenient to allocate a new stack with reserved memory

Checklist

documentation is added or updated

levitte · 2017-10-19T18:30:08Z

doc/man3/DEFINE_STACK_OF.pod

@@ -31,7 +32,7 @@ sk_TYPE_dup, sk_TYPE_deep_copy, sk_TYPE_set_cmp_func - stack container
 TYPE *sk_TYPE_value(const STACK_OF(TYPE) *sk, int idx);
 STACK_OF(TYPE) *sk_TYPE_new(sk_TYPE_compfunc compare);
 STACK_OF(TYPE) *sk_TYPE_new_null(void);
- int sk_TYPE_reserve(STACK_OF(TYPE) *sk, size_t n);
+ int sk_TYPE_reserve(STACK_OF(TYPE) *sk, int n);


Because that's what OPENSSL_sk_reserve accepts. It's harmonization with code.

Ah! Thanks for the reminder

Internally the stack structure uses an int for the number of elements present. This is exposed by the call sk_TYPE_num which returns an int. Changing to a size_t makes sense to me but it is an API change.

In fact, next time when API could be changed, IMHO the new APIs of stack could be squashed into one, as something like sk_TYPE_new(int n, sk_TYPE_compfunc compare). If n == 0, then it equals to sk_TYPW_new_null...

Just to clarify, this is wrong in the current documentation, and the documentation is fixed. Maybe you can move that fix to a different pull request so we can merge that to other branches?

Or a separated commit inside this PR? and thus you can cherry-pick that commit...

That works too of course

levitte · 2017-10-19T18:30:42Z

doc/man3/DEFINE_STACK_OF.pod

@@ -53,6 +54,7 @@ sk_TYPE_dup, sk_TYPE_deep_copy, sk_TYPE_set_cmp_func - stack container
                                   sk_TYPE_freefunc freefunc);
 sk_TYPE_compfunc (*sk_TYPE_set_cmp_func(STACK_OF(TYPE) *sk,
                                         sk_TYPE_compfunc compare));
+ STACK_OF(TYPE) *sk_TYPE_new_reserve(int n);


Why not size_t?

Same as above.

levitte · 2017-10-19T18:38:02Z

crypto/stack/stack.c

+    if (!OPENSSL_sk_reserve(st, n)) {
+        OPENSSL_sk_free(st);
+        return NULL;
+    }


Side note: in other places, we've used aggregate conditions, like this:

if ((st = OPENSSL_new_null()) == NULL || !OPENSSL_sk_reserve(st, n)) { OPENSSL_sk_free(st); return NULL; }

Your choice if you want to do the same

Okay, aggregate conditions work for me

paulidale · 2017-10-19T21:32:50Z

doc/man3/DEFINE_STACK_OF.pod

+sk_TYPE_new_reserve() allocates a new stack with no comparison function, but
+additional memory in the new stack is allocated based on B<n>. Next B<n> calls
+to sk_TYPE_insert(), sk_TYPE_push() or sk_TYPE_unshift() will not fail or
+cause memory to be allocated or reallocated.


How about:

sk_TYPE_new_reserve() allocates a new empty stack with no comparison function. The new stack will have additional memory allocated to hold B elements. The next B calls to sk_TYPE_insert(), sk_TYPE_push() or sk_TYPE_unshift() will not fail or cause memory to be allocated or reallocated.

Yes, this is better

paulidale · 2017-10-19T21:35:18Z

doc/man3/DEFINE_STACK_OF.pod

@@ -213,6 +220,9 @@ index is out of range.
 sk_TYPE_new() and sk_TYPE_new_null() return an empty stack or B<NULL> if
 an error occurs.

+sk_TYPE_new_reserve() returns a stack with allocation of the required
+memory or B<NULL> if an error occurs.


I think this could be included with sk_TYPE_new rather than being spelt out separately. That there is additional memory allocated is mentioned in the description and isn't relevant to the returns section.

sk_TYPE_new(), sk_TYPE_new_null() and sk_TYPE_new_reserve() return an empty stack or B if an error occurs.

Then you might need to indicate the difference of returned stack among those new APIs, so how about:

sk_TYPE_new(), sk_TYPE_new_null() and sk_TYPE_new_reserve() return an empty stack with or without preallocated memory, or B if an error occurs.

That's okay too. The preallocation is mentioned above in the function description. I don't see a hard requirement for it to be mentioned again here.

Hmmm, sounds reasonable.

InfoHunter · 2017-10-20T02:08:42Z

Comments are addressed by the new commits...

paulidale · 2017-10-20T03:16:15Z

doc/man3/DEFINE_STACK_OF.pod

@@ -101,6 +103,11 @@ or sk_TYPE_unshift() will not fail or cause memory to be allocated
 or reallocated. If B<n> is zero, any excess space allocated in the
 B<sk> structure is freed. On error B<sk> is unchanged.

+sk_TYPE_new_reserve() allocates a new stack with no comparison function.
+The new stack will have additional memory allocated to hold B elements.


Shouldn't that be B<n> ?

paulidale

Subject to the one comment.

InfoHunter · 2017-10-20T05:16:02Z

Fixed. The second commit should not be squashed

dot-asm · 2017-10-20T21:06:28Z

Here is idea. How about "flat" structure. I mean instead of allocating stack_st and allocating void *data[reservation] allocate stack_st + void *data[reservation] with stack_st->data pointing at stack_st + sizeof(stack_st).

richsalz · 2017-10-20T21:19:59Z

I thought we outgrew that kind of stuff :) Less flippantly, don't we have to worry about alignment? IT's hard to get this kind of thing correct in portable code. It would be nice to avoid the fragmentation for fixed-sized stacks but I'm not sure the tradeoff is worth it.

dot-asm · 2017-10-20T21:25:02Z

don't we have to worry about alignment?

sizeof(structure) is always multiple of "largest" member type. This means that if there is a pointer member, then you can always "attach" array of pointers.

levitte · 2017-10-21T04:24:04Z

I don't think that the aggregate structure @dot-asm proposes is a good idea. Consider if the stack needs to grow, how would you realloc the data?

InfoHunter · 2017-10-21T13:14:14Z

Yes, I think this (stack_st + void *data[reservation]) could work for fix-sized stack, but not for generic usage. Even one reserves memory at creating a stack, he/she might also extend the stack later...

levitte · 2017-10-21T15:20:04Z

A fixed-size stack is a glorified array, then you should really just have an array with an integer for growth index ;-) After all, the point with our stack implementation is the ability to grow, that's what makes it different from a simple array...

dot-asm · 2017-10-21T16:03:12Z

Recall that reservations are for cases when caller actually knows size in advance. Yes, it's glorified array, but you have no choice, because callee wants it this way. As simplest mental exercise imagine function in question was called sk_TYPE_new_reserved... As for growing. Two options: a) return error (it's not unreasonable for reserved stack); b) allocate larger array and adjust stack_st.data to point at newly allocated array (the originally "attached" array will hang around wasted, but will be dutifully freed in free(stack_st)).

[As another mental exercise imagine you have subroutine adding two numbers, would you implement subroutine adding 42 to one number? I mean additional interfaces ought to add some value...]

levitte · 2017-10-21T16:11:15Z

So imagine someone passing such a reserved stack to another unsuspecting function that wants to add more items. Generically speaking, that would be a bad thing, and I don't think that's an acceptable change of stack semantics. As for adjusting stack_st.data and leave the original data chunk there... well, if that data chunk is small enough, what's a few bytes, but if someone reserves a large number of slots that makes the loss significant enough.

I'm not sure the gain is worth the possible cost.

levitte · 2017-10-21T16:11:43Z

(also, I'm not sure what the gain really is. One less OPENSSL_zalloc()?)

dot-asm · 2017-10-21T16:20:23Z

One less OPENSSL_zalloc()?

Less fragmentation, better cache locality, less contention on MT malloc lock...

InfoHunter · 2017-10-21T16:58:09Z

Recall that reservations are for cases when caller actually knows size in advance. Yes, it's glorified array, but you have no choice, because callee wants it this way.

Another use-case for stack reservation would be: a user doesn't know the size in advance, but the user does know there must be at least some size in the stack (Though I don't know if this case is considered in the initial motivation of the design). For instance, taking RSA as an example, someone may create a stack and always reserve 2 slots to contain the primes when creating the RSA object, since there must be at least 2 primes there but at this stage he/she doesn't know how many additional primes will be there... (Though the real RSA-related code is not implemented in this way, but this just could be an example...)

dot-asm · 2017-10-22T19:55:21Z

a user doesn't know the size in advance,

Then this may be not for such user.

imagine someone passing such a reserved stack to another unsuspecting function that wants to add more items.

Then user shouldn't create reserved stack. But use pair of new + reserve calls. But once again, it's an a) option. But there is b)...

levitte · 2017-10-22T21:17:56Z

imagine someone passing such a reserved stack to another unsuspecting function that wants to add more items.

Then user shouldn't create reserved stack. But use pair of new + reserve calls.

If we're going to give this function such different semantics, then it should have a different. Having "reserve" mean different things depending on exactly which function was called is hugely misleading.

But once again, it's an a) option. But there is b)...

paulidale · 2017-10-22T21:23:15Z

I'm also against allocating the reserved slots as part of the structure.

I've seen and written C++ code that reserved space in a std::vector after creation and then needed that to expand. This is the same situation. Do not, however, reserve an additional slot each time you add one item ;)

dot-asm · 2017-10-22T21:35:41Z

function was called is hugely misleading

As already mentioned, imagine that was called new_reserved [note additional "d"]. :-)

levitte · 2017-10-22T21:42:38Z

As already mentioned, imagine that was called new_reserved [note additional "d"]. :-)

Seriously, that's not different enough.

dot-asm · 2017-10-22T21:59:15Z

new_immutable then. But let me put it this way. In the essence what I'm trying to say is that additional public APIs ought to add real value, and mechanical combination of two calls doesn't really cut it. If aggregated/immutable is resenting, what would be more meaningful is to introduce subroutine that will work as replacement for both new flavours and reserve in the next major release...

paulidale · 2017-10-22T22:21:59Z

I do wonder why there are both OPENSSL_sk_new and OPENSSL_sk_new_null already. The latter passes a NULL to the former. It doesn't seem worthy to be an exported API.

I'm also a little dubious about the merit of the new_reserve call.

I don't see any need for a new_immutable call. Create an array instead.

I have thought about a call to make a stack immutable after creation. I.e. I'm done adding stuff to this and it is read only from here.

InfoHunter · 2017-10-23T07:11:08Z

In the essence what I'm trying to say is that additional public APIs ought to add real value, and mechanical combination of two calls doesn't really cut it.

To make the library more convenient to use is just the value here. Furthermore, if the sk_XXX_reserve API is always called right after a sk_XXX_new_null is called (as the code I investigated, all use cases are this), then there should not be a standalone sk_reserve API at all, the separation of new and reserve has no benefits but only bothering the users.

And yes, there should be one new API to cover all functionality for current new, reserve and compfunc setting stuffs in next API changeable release...

dot-asm · 2017-10-23T08:11:19Z

there should be one new API to cover all functionality for current new, reserve and compfunc setting stuffs in next API changeable release...

It has to be in next major release if we want to re-use name[s]. But if we add function with new name with intention to remove old names, then it can be done now. New name is actually more appropriate, because it's considered good style if old names hang around in intermediate major release. [Though it can as well be arranged with macros, i.e. you can compile program but there are no exported symbols.]

Also note that sk_TYPE_reserve was just added, it wasn't released, so one can still replace it. To summarize, it would be more meaningful to design function that replaces all three, but keep original two till next major release, and discard sk_TYPE_reserve.

[As for "flat" structure. Note that min_nodes is as low as 4. Each allocation has an overhead, and 4 pointers is somewhat wasteful. One can argue that initial minimum allocation can as well be flat...]

InfoHunter · 2017-10-23T17:51:23Z

Fine. I changed OPENSSL_sk_new_reserve to two parameters to cover all new related functionality. And OPENSSL_sk_reserve is not removed in this PR since that requires changes of other code, I intend to do that in a separate PR...

dot-asm · 2017-10-24T09:51:21Z

I not convinced that we want to remove the reserve call. It is useful in its own right. That we only use it after just means we've not looked at other places or written new code using it yet.

You mean when you'd expect callee to make the reservation, right? I mean you want somebody else to have an opinion about reservation. Like callee would reserve place for additional elements it wants to add... Yeah, it makes sense...

InfoHunter · 2017-10-24T16:45:04Z

new commits pushed...

dot-asm · 2017-10-24T20:08:56Z

doc/man3/DEFINE_STACK_OF.pod


-sk_TYPE_new_null() allocates a new empty stack with no comparison function.
+sk_TYPE_new_null() allocates a new empty stack with no comparison function. This
+function is equivalent to sk_TYPE_new_reserve(NULL, 0).

 sk_TYPE_reserve() allocates additional memory in the B<sk> structure
 such that the next B<n> calls to sk_TYPE_insert(), sk_TYPE_push()
 or sk_TYPE_unshift() will not fail or cause memory to be allocated
 or reallocated. If B<n> is zero, any excess space allocated in the
 B<sk> structure is freed. On error B<sk> is unchanged.


On related side note. Sentence before last one seems misleading. Excess space is apparently freed whenever current data + reservation is smaller than st.num_alloc. In other words it's not specific to case of n being zero. Ping @paulidale.

Yes, the excess space is freed in this circumstance too. However, the guarantee that the next n additions won't cause memory allocation is true.

I don't see a need to document this internal behaviour -- the stack will still function as expected. If this were documented, it would unnecessarily restrict any future re-implementation of the stack.

In other words the sentence in question should be simply omitted, right?

I think so. PR #4582 is created to remove that sentence, to avoid 're-approval' of this PR...

I'm not sure removal is required. Being able to free up excess space seems like a reasonable thing to ask for, even without knowing anything about the internals of the stack data structure.

Then the Note section might be a better place to put this. In fact, I feel it would be better either to add a new standalone API to handle the 'free' job of the excess spaces to avoid misleading, e.g. OPENSSL_sk_shrink...

(First time I saw OPENSSL_sk_reserve and I thought if I passed a 0 of n, it would do nothing, just as what malloc-like functions do. But it's not...)

paulidale

One documentation nit about a missing the.
Approved once this is added.

paulidale · 2017-10-25T02:41:18Z

doc/man3/DEFINE_STACK_OF.pod


 sk_TYPE_reserve() allocates additional memory in the B<sk> structure
 such that the next B<n> calls to sk_TYPE_insert(), sk_TYPE_push()
 or sk_TYPE_unshift() will not fail or cause memory to be allocated
 or reallocated. If B<n> is zero, any excess space allocated in the
 B<sk> structure is freed. On error B<sk> is unchanged.

-sk_TYPE_set_cmp_func() sets the comparison function of B<sk> to B<compar>.
+sk_TYPE_new_reserve() allocates a new stack. The new stack will have additional
+memory allocated to hold B<n> elements if B<n> is positive. Next B<n> calls to


The next B<n> calls

This is a combination of sk_new and sk_reserve, to make it more convenient to allocate a new stack with reserved memory and comaprison function (if any).

<compar> to <compare> to match the var name in function prototype

InfoHunter · 2017-10-25T07:30:59Z

The 'The' is added...

That sentence is very internal, as discussed in: openssl#4559 (comment)

paulidale · 2017-10-25T22:21:32Z

You mean when you'd expect callee to make the reservation, right? I mean you want somebody else to have an opinion about reservation. Like callee would reserve place for additional elements it wants to add... Yeah, it makes sense...

Think of it as a hint to the data type that a number of adds are about to happen. We could live without it, but we could be more efficient with it.

paulidale · 2017-10-25T22:28:33Z

I can't merge this. I'm getting missing reviewed-by headers.

levitte

I think this will help, @paulidale

This is a combination of sk_new and sk_reserve, to make it more convenient to allocate a new stack with reserved memory and comaprison function (if any). Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4559)

Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4559)

<compar> to <compare> to match the var name in function prototype Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4559)

paulidale · 2017-10-25T23:36:56Z

Yes, that fixed it. Merged thanks everyone.

InfoHunter · 2017-10-26T10:34:53Z

And someone can cherry-pick the doc-nits-fix commits to other branch now...

<compar> to <compare> to match the var name in function prototype Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4559) (cherry picked from commit d9c989f)

levitte · 2017-10-26T10:49:24Z

doc-nits-fix backported to 1.1.0: 2e6d51d

InfoHunter mentioned this pull request Oct 19, 2017

Support multi-prime RSA (RFC 8017) #4241

Closed

2 tasks

levitte requested changes Oct 19, 2017

View reviewed changes

paulidale reviewed Oct 19, 2017

View reviewed changes

paulidale reviewed Oct 20, 2017

View reviewed changes

paulidale approved these changes Oct 20, 2017

View reviewed changes

InfoHunter force-pushed the new-reserve branch from d252b76 to a334256 Compare October 20, 2017 05:12

InfoHunter force-pushed the new-reserve branch from a334256 to c85c7fb Compare October 23, 2017 17:37

InfoHunter force-pushed the new-reserve branch 2 times, most recently from 0c3ef78 to 234547d Compare October 24, 2017 16:44

dot-asm reviewed Oct 24, 2017

View reviewed changes

dot-asm approved these changes Oct 24, 2017

View reviewed changes

dot-asm added the approval: review pending This pull request needs review by a committer label Oct 24, 2017

paulidale approved these changes Oct 25, 2017

View reviewed changes

InfoHunter added 3 commits October 25, 2017 15:27

Add sk_TYPE_new_reserve() function

50b179d

This is a combination of sk_new and sk_reserve, to make it more convenient to allocate a new stack with reserved memory and comaprison function (if any).

Fix mismatch of function prototype and document

0e91f65

Fix doc-nits in doc/man3/DEFINE_STACK_OF.pod

86d6a4c

<compar> to <compare> to match the var name in function prototype

InfoHunter force-pushed the new-reserve branch from 234547d to 86d6a4c Compare October 25, 2017 07:27

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 25, 2017

InfoHunter added a commit to InfoHunter/openssl that referenced this pull request Oct 25, 2017

Delete misleading sentence in doc of sk_TYPE_reserve

3186483

That sentence is very internal, as discussed in: openssl#4559 (comment)

InfoHunter mentioned this pull request Oct 25, 2017

Delete misleading sentence in doc of sk_TYPE_reserve #4582

Closed

2 tasks

levitte approved these changes Oct 25, 2017

View reviewed changes

levitte pushed a commit that referenced this pull request Oct 25, 2017

Fix mismatch of function prototype and document

62f45e3

Reviewed-by: Paul Dale <paul.dale@oracle.com> Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #4559)

paulidale closed this Oct 25, 2017

InfoHunter deleted the new-reserve branch October 26, 2017 10:34

InfoHunter mentioned this pull request Oct 26, 2017

Simplify the stack reservation #4592

Closed

Add sk_TYPE_new_reserve() function #4559

Add sk_TYPE_new_reserve() function #4559

Conversation

InfoHunter commented Oct 19, 2017

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InfoHunter commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale left a comment

Choose a reason for hiding this comment

InfoHunter commented Oct 20, 2017

dot-asm commented Oct 20, 2017

richsalz commented Oct 20, 2017

dot-asm commented Oct 20, 2017

levitte commented Oct 21, 2017

InfoHunter commented Oct 21, 2017

levitte commented Oct 21, 2017

dot-asm commented Oct 21, 2017

levitte commented Oct 21, 2017

levitte commented Oct 21, 2017

dot-asm commented Oct 21, 2017

InfoHunter commented Oct 21, 2017

dot-asm commented Oct 22, 2017

levitte commented Oct 22, 2017

paulidale commented Oct 22, 2017

dot-asm commented Oct 22, 2017

levitte commented Oct 22, 2017

dot-asm commented Oct 22, 2017

paulidale commented Oct 22, 2017

InfoHunter commented Oct 23, 2017

dot-asm commented Oct 23, 2017

InfoHunter commented Oct 23, 2017

dot-asm commented Oct 24, 2017

InfoHunter commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale left a comment

Choose a reason for hiding this comment

paulidale Oct 25, 2017 • edited by levitte

Choose a reason for hiding this comment

InfoHunter commented Oct 25, 2017

paulidale commented Oct 25, 2017

paulidale commented Oct 25, 2017

levitte left a comment

Choose a reason for hiding this comment

paulidale commented Oct 25, 2017

InfoHunter commented Oct 26, 2017

levitte commented Oct 26, 2017

paulidale Oct 25, 2017 •

edited by levitte