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.

Sign up for GitHub

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

Jump to bottom

ecp_sm2p256-armv8.pl: Copy the argument handling from ecp_nistz256 #21877

Closed
wants to merge 1 commit into from
Closed

ecp_sm2p256-armv8.pl: Copy the argument handling from ecp_nistz256 #21877

wants to merge 1 commit into from

Conversation

t8m
Copy link
Member

@t8m t8m commented Aug 28, 2023

Fixes #21874
Fixes #21876

@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing labels Aug 28, 2023
@levitte
Copy link
Member

levitte commented Aug 28, 2023

I would mention in the commit message that popping the arguments to get the output file is more robust.

levitte
levitte approved these changes Aug 28, 2023
View reviewed changes
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

LGTM with an amended commit message

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Aug 28, 2023
@t8m
ecp_sm2p256-armv8.pl: Copy the argument handling from ecp_nistz256-ar…
77af444 
…mv8.pl

Popping the $output argument is more robust and it also needs to be
placed in double quotes to handle spaces in paths.

Fixes openssl#21874
Fixes openssl#21876
@t8m
Copy link
Member Author

t8m commented Aug 28, 2023

@levitte commit message adjusted

@levitte levitte mentioned this pull request Aug 28, 2023
Closed
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Aug 28, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

Okay, this is the way it's done in all the other *-armv8.pl scripts, so it's good

@tom-cosgrove-arm tom-cosgrove-arm removed the approval: review pending This pull request needs review by a committer label Aug 28, 2023
@levitte
Copy link
Member

levitte commented Aug 29, 2023

Okay, this is the way it's done in all the other *-armv8.pl scripts, so it's good

Historically, we've had a mix of popping the output argument from the end of the argument list, or shifting from the start to find it... or none of them and always sending the output the STDOUT. This very much depended on when those scripts were authored and how the method for output had developed at that time.

I did some sort of streamlining effort years ago, to at least make sure that they would all take an output file as last argument. It's possible, though, that they haven't all been streamlined to use the popping method. That could be a worthy endeavor if someone is inclined, as it would probably stop new perlasm scripts from being inspired by other scripts that don't use that methid.

@tom-cosgrove-arm
Copy link
Contributor

@levitte If you're happy with the commit message can you update the approval labels?

levitte
levitte approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

Happy

@levitte levitte added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels Aug 30, 2023
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Aug 31, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member Author

t8m commented Aug 31, 2023

Merged to the master branch. Thank you for the reviews.

@t8m t8m closed this Aug 31, 2023
openssl-machine pushed a commit that referenced this pull request Aug 31, 2023
@t8m
ecp_sm2p256-armv8.pl: Copy the argument handling from ecp_nistz256-ar…
8a7f30e 
…mv8.pl

Popping the $output argument is more robust and it also needs to be
placed in double quotes to handle spaces in paths.

Fixes #21874
Fixes #21876

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from #21877)
heygauri pushed a commit to heygauri/openssl that referenced this pull request Sep 1, 2023
@t8m @heygauri
ecp_sm2p256-armv8.pl: Copy the argument handling from ecp_nistz256-ar…
729ef33 
…mv8.pl

Popping the $output argument is more robust and it also needs to be
placed in double quotes to handle spaces in paths.

Fixes openssl#21874
Fixes openssl#21876

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from openssl#21877)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte approved these changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
4 participants
@t8m @levitte @tom-cosgrove-arm @openssl-machine