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

Fix #38, Handle lseek error. #39

Closed
wants to merge 2 commits into from

Conversation

zanzaben
Copy link
Contributor

@zanzaben zanzaben commented Mar 3, 2021

Describe the contribution
Fixes #38
Checks if lseek returns an error and reports it.

Fixes #40
Use exit(1)

Testing performed
Build and run unit test

Expected behavior changes
No impact to behavior

System(s) tested on
Ubuntu 20.04

Contributor Info - All information REQUIRED for consideration of pull request
Alex Campbell GSFC

@astrogeco
Copy link
Contributor

astrogeco commented Mar 3, 2021

CCB 2021-03-03 APPROVED with CHANGES

  • add exit 1 instead since exit 0 means success, open new issue
  • Conversation about using strerror
  • Does it make sense to check against the desired offset instead?
  • Use off_t instead of int when checking the output

Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

Requesting a couple more fixups, otherwise good. See comments inline.

cfe_ts_crc.c Outdated Show resolved Hide resolved
cfe_ts_crc.c Outdated Show resolved Hide resolved
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

Sorry missed something in my previous skim-through. Hopefully this is the last nitpick!

if (offsetReturn != skipSize)
{
printf("\ncfe_ts_crc error: lseek failed!\n");
printf("%s\n", strerror(offsetReturn));
Copy link
Contributor

Choose a reason for hiding this comment

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

strerror() takes an errno, not the return code (the latter is -1 by definition).

So it should be strerror(errno) ... OR just use perror() routine instead. That just simplifies when you want to print the current errno.

We should also do this EVERY time we do exit(1) - not just here.

I do recommend using this function when all you want to do is print the error in the simplest way possible. It's quick and easy. See: https://man7.org/linux/man-pages/man3/perror.3p.html

@zanzaben
Copy link
Contributor Author

zanzaben commented Mar 4, 2021

Moving this to #42

@zanzaben zanzaben closed this Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Report error and use exit(1) for errors and exit(0) for success Check for and report lseek failures
3 participants