Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UI functionality to duplicate the user data #3575

Closed
wants to merge 2 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented May 28, 2017

This can be used by engines that need to retain the data for a longer time
than just the call where this user data is passed.

Checklist
  • documentation is added or updated

@levitte levitte added the branch: master Merge to master branch label May 28, 2017
@levitte
Copy link
Member Author

levitte commented May 28, 2017

@dwmw2, I believe you're interested in this

@dwmw2
Copy link
Contributor

dwmw2 commented May 28, 2017

Staring at it on my.phone probably doesn't help but... Where does the 'destroy' function originally come from? Shouldnt it be passed in from the app as an additional argument to the 'set duplicator' call. Dup and free functions are a pair. (And if it's recounting are just Inc and dec)

@levitte
Copy link
Member Author

levitte commented May 28, 2017

Very good point, I'll fix

@dwmw2
Copy link
Contributor

dwmw2 commented May 28, 2017

I might even go so far as to suggest that the add_user_data function should take all three. To add user data without the dup/free funcs you should need to explicitly pass NULL

@levitte
Copy link
Member Author

levitte commented May 28, 2017

I went the other way and made the destructor a UI_METHOD function as well.

@dwmw2
Copy link
Contributor

dwmw2 commented May 28, 2017

... and the UI data so added is always destroyed by OpenSSL for consistency.

@dwmw2
Copy link
Contributor

dwmw2 commented May 28, 2017

LGTM

UI_method_set_prompt_constructor, UI_method_set_ex_data,
UI_method_get_opener, UI_method_get_writer, UI_method_get_flusher,
UI_method_get_reader, UI_method_get_closer,
UI_method_get_data_duplicator, UI_method_get_data_destructor,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update HISTORY section?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

UI_add_user_data, UI_get0_user_data, UI_get0_result, UI_process,
UI_ctrl, UI_set_default_method, UI_get_default_method, UI_get_method,
UI_set_method, UI_OpenSSL, UI_null - user interface
UI_add_user_data, UI_dup_user_data, UI_get0_user_data, UI_get0_result,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HISTORY?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return old_data;
}

int UI_dup_user_data(UI *ui, void *user_data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this really be called UI_set1_user_data()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be inconsistent with the rest of the UI API. You might want to argue that the API should be "modernised", but that's for 1.2.0, not for now.

Background: the whole set0 / set1 semantics came after UI, as far as I recall, or was at least not settled and well known throughout the whole team at the time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about add1 then? There are quite a few instances of that in the public APIs already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about looking at the rest of ui.h first? There is quite a number of add/dup pairs, why should this particular function differ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Ok.

This can be used by engines that need to retain the data for a longer time
than just the call where this user data is passed.
@levitte
Copy link
Member Author

levitte commented May 31, 2017

Merged. 545360c

Thanks!

@levitte levitte closed this May 31, 2017
levitte added a commit that referenced this pull request May 31, 2017
This can be used by engines that need to retain the data for a longer time
than just the call where this user data is passed.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #3575)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants