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

Fix #36, handle read failure #37

Closed
wants to merge 1 commit into from
Closed

Fix #36, handle read failure #37

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion cfe_ts_crc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,12 @@ int main(int argc, char **argv)
while (done == 0)
{
readSize = read(fd, buffer, 100);
fileCRC = CalculateCRC(buffer, readSize, fileCRC);
if (readSize < 0)
{
printf("\ncfe_ts_crc error: file read failed!\n");
exit(1);
}
fileCRC = CalculateCRC(buffer, (uint32)readSize, fileCRC);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that a cast to (uint32) was added here? Shouldn't be needed.
Also see my other comment in the other PR that the return type of read() is ssize_t, not int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done because CalculateCRC is expecting a uint32

Copy link
Contributor

Choose a reason for hiding this comment

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

But it will convert implicitly - and forcing a conversion can have unintended consequences. In reality the CalculateCRC() functions should take a size_t anyway.

Note that C has very specific rules about when and how implicit type conversions are done - and things (almost) always just work. When it doesn't just work you'll get a compiler warning about it, but a cast can hide that warning because it is literally telling the compiler "I meant to do that". Hence why its not advised to put them in unless there is a real reason why you need to tell the compiler that you meant to do that. In this case, I don't see any reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note - I'm not suggesting changing CalculateCRC to size_t here - just leave it alone - it doesn't matter. My only concern is the extra cast - should not do this.

fileSize += readSize;
if (readSize != 100)
done = 1;
Expand Down