GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — check if elffile is a file, make exception more descriptive by tmnku · Pull Request #193 · linuxdeploy/linuxdeploy

Conversation

@tmnku

@tmnku tmnku commented Feb 12, 2022

Copy link
Copy Markdown

I accidentally passed a directory instead of an executable along --executable. The additional check and printing out the filename would have helped me find the error more easily.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TheAssassin reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
TheAssassin
TheAssassin reviewed Feb 12, 2022
View reviewed changes

@TheAssassin TheAssassin left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks for your contribution, looks really good. I left a comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/core/elf_file.cpp Outdated
// check if file exists
if (!bf::exists(path))
throw ElfFileParseError("No such file or directory: " + path.string());
if (!bf::exists(path) || !bf::is_regular_file(path))

@TheAssassin TheAssassin Feb 12, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think exists is redundant, isn't it? I haven't checked the docs.

We should switch to C++17...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TheAssassin TheAssassin Feb 12, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Indeed, exists is redundant here. Please fix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
TheAssassin
TheAssassin requested changes Feb 12, 2022
View reviewed changes
Comment thread src/core/elf_file.cpp Outdated
// check if file exists
if (!bf::exists(path))
throw ElfFileParseError("No such file or directory: " + path.string());
if (!bf::exists(path) || !bf::is_regular_file(path))

@TheAssassin TheAssassin Feb 12, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Indeed, exists is redundant here. Please fix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
TheAssassin
TheAssassin requested changes Feb 13, 2022
View reviewed changes

@TheAssassin TheAssassin left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unit tests fail due to this change, please check and fix them if needed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tmnku

tmnku commented Feb 13, 2022

Copy link
Copy Markdown
Author

Thanks, the tests should pass now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TheAssassin
TheAssassin reviewed Feb 20, 2022
View reviewed changes
Comment thread tests/core/test_elf_file.cpp

TEST_F(ElfFileTest, checkInvalidElfHeaderOnEmptyFile) {
expectThrowsElfFileErrorInvalidElfHeader("/dev/null");
expectThrowsElfFileErrorInvalidElfHeader(EMPTY_FILE_PATH);

@TheAssassin TheAssassin Feb 20, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good idea!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@tmnku tmnku Feb 21, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The change was needed since is_regular_file returns false for /dev/null.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tmnku
tmnku requested a review from TheAssassin February 21, 2022 07:03
TheAssassin
TheAssassin approved these changes May 4, 2022
View reviewed changes
Tilman added 2 commits May 4, 2022 03:12
Tilman @TheAssassin
check if elffile is a file, make exception more descriptive
9158554
Tilman @TheAssassin
fix tests
5782892
@TheAssassin
TheAssassin force-pushed the check_elffile_is_file branch from 0134c88 to 5782892 Compare May 4, 2022 01:12
@TheAssassin
Merge branch 'master' into check_elffile_is_file
d416564
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheAssassin TheAssassin TheAssassin approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@tmnku @TheAssassin
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.