Skip to content

Fixed validation of full (absolute) filename#910

Merged
uweseimet merged 5 commits intodevelopfrom
fix_file_validation
Oct 12, 2022
Merged

Fixed validation of full (absolute) filename#910
uweseimet merged 5 commits intodevelopfrom
fix_file_validation

Conversation

@uweseimet
Copy link
Copy Markdown
Contributor

@uweseimet uweseimet commented Oct 11, 2022

No description provided.

rdmark
rdmark reviewed Oct 11, 2022
View reviewed changes
src/raspberrypi/rascsi/rascsi_executor.cpp

disk->Open(filepath);
if (!disk->FileExists(filepath)) {
return context.ReturnLocalizedError(LocalizationKey::ERROR_FILE_OPEN, initial_filename);
Copy link
Copy Markdown
Member

@rdmark rdmark Oct 11, 2022

Choose a reason for hiding this comment

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

Should we remove the e.get_msg() argument from the ReturnLocalizedError call on line 589 below? Since you changed the error string to only take one positional string.

Copy link
Copy Markdown
Contributor Author

@uweseimet uweseimet Oct 11, 2022

Choose a reason for hiding this comment

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

@rdmark You are right, I missed that. Wouldn't have done harm, but is wrong. Just fixed it.

@uweseimet
Copy link
Copy Markdown
Contributor Author

uweseimet commented Oct 11, 2022 •
edited
Loading

@rdmark I stumbled upon this as part of simplifying the filename handling as part of #722 (for the next release). Currently it is quite complicated due to the use of legacy classes instead of the standard C++ string class.
It might be a good idea to keep this PR open for one or two days, just in case I find something else while working on the new code.

@rdmark
Copy link
Copy Markdown
Member

rdmark commented Oct 11, 2022

@uweseimet So if the purpose here is to enable future improvements, shall we wait until after the release to merge this code?

@uweseimet
Copy link
Copy Markdown
Contributor Author

uweseimet commented Oct 11, 2022 •
edited
Loading

@rdmark No, because this is a bug (compared with 22.08.01), not just an improvement. Otherwise I would not have raised this PR, because currently I only create PRs for bugs that would affect the next release.
Edit: I may not have been clear enough: I found this issue while testing new code, which will not become part of the 22.10 release, and while adding new unit tests. But I noticed that the bug I found was also present in the current develop branch, thus this PR.

@rdmark
Copy link
Copy Markdown
Member

rdmark commented Oct 11, 2022

I understand your intentions now. The change seems reasonable so I'll accept and let you merge when ready.

@uweseimet
Copy link
Copy Markdown
Contributor Author

uweseimet commented Oct 11, 2022

@rdmark OK. In case another change is needed I will commit and then ask again for explicit approval. If no other change is required (further testing of the new code has not yet revealed anything new) I will merge tomorrow or so. I case you need the merge earlier, just go ahead.

@uweseimet
Copy link
Copy Markdown
Contributor Author

uweseimet commented Oct 11, 2022

Fixed a missing CR/LF in the rasctl output.

Before:

  SCLP  Parameter support
        Default parameters: cmd=lp -oraw %fReserved device IDs: 7

After:

  SCLP  Parameter support
        Default parameters: cmd=lp -oraw %f
Reserved device IDs: 7

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Oct 11, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@uweseimet
Copy link
Copy Markdown
Contributor Author

uweseimet commented Oct 12, 2022

@rdmark No further issues so far, except for the fixed typo in the previous commit.

@rdmark
Copy link
Copy Markdown
Member

rdmark commented Oct 12, 2022

@uweseimet That's great. I suggest we merge this PR today, and then call an informal code freeze until the release is tagged this weekend. What do you think?

@uweseimet
Copy link
Copy Markdown
Contributor Author

uweseimet commented Oct 12, 2022

@rdmark I thought we already had a code freeze, execpt for bug fixes. Anyway, I will merge soon.

@uweseimet uweseimet merged commit 0c8d54f into develop Oct 12, 2022
@uweseimet uweseimet deleted the fix_file_validation branch October 12, 2022 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rdmark rdmark rdmark approved these changes

@akuker akuker Awaiting requested review from akuker akuker is a code owner

@erichelgeson erichelgeson Awaiting requested review from erichelgeson

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@uweseimet @rdmark