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

Misc apps fixes [2020-05-28] #11983

Closed
wants to merge 3 commits into from
Closed

Conversation

levitte
Copy link
Member

@levitte levitte commented May 28, 2020

make_config_name() is a duplicate of CONF_get1_default_config_file(), so drop it.

apps/progs.pl has strange results when it's running in a national locale. For example, w isn't matched by the regular expression [a-z] in the Swedish locale (which means passwd_main isn't matched, for example). To avoid that sort of surprise, we set the "C" locale.

@levitte levitte added branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels May 28, 2020
t8m
t8m previously approved these changes May 28, 2020
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

LGTM

@richsalz
Copy link
Contributor

Explicitly running in the "C" locale seems like it will be bad for some users, e.g., @beldmit and @InfoHunter and their countrymen. Are you sure that's the right thing to do, rather than add a NOTE (sic) in the documentation?

@beldmit
Copy link
Member

beldmit commented May 28, 2020

Explicit setting locale to "C" is often a way to find out an understandable (or at least googlable) diagnostic output in Russia :)

@levitte
Copy link
Member Author

levitte commented May 28, 2020

In my case, not running under the "C" locale was bad. We are, after all, trying to match ASCII characters, so the "C" locale is the best way to ensure [a-z] matches them all.

apps/openssl.c Outdated Show resolved Hide resolved
@levitte
Copy link
Member Author

levitte commented May 28, 2020

I've dropped the locale change in apps/progs.pl, couldn't reproduce what went wrong when I tried again.

@levitte levitte dismissed t8m’s stale review May 28, 2020 15:01

Re-approval needed

@t8m t8m added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Jun 2, 2020
@t8m
Copy link
Member

t8m commented Jun 2, 2020

Approved 5 days ago by Bernd already.

@levitte
Copy link
Member Author

levitte commented Jun 2, 2020

Merged

e306f83 APPS: Remove make_config_name, use CONF_get1_default_config_file instead

@levitte levitte closed this Jun 2, 2020
openssl-machine pushed a commit that referenced this pull request Jun 2, 2020
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #11983)
@levitte levitte deleted the fix-apps-20200528 branch June 6, 2020 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants