Update lseek.c #51
Open
Update lseek.c #51
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis PR extends the lseek loadable builtin functionality by adding support for SEEK_SET and SEEK_END seek types in addition to the existing SEEK_CUR behavior. The implementation adds an optional third parameter to specify the seek type, defaulting to SEEK_CUR for backward compatibility. Sequence diagram for lseek command executionsequenceDiagram
actor User
participant lseek_builtin as lseek_builtin
participant lseek_main as lseek_main
participant lseek as lseek
User->>lseek_builtin: Execute lseek <FD> <REL_OFFSET> [<SEEK_TYPE>]
lseek_builtin->>lseek_main: Parse arguments
alt Valid arguments
lseek_main->>lseek: Call lseek with SEEK_TYPE
alt SEEK_TYPE is SEEK_SET
lseek-->>lseek_main: Move FD to offset from start
else SEEK_TYPE is SEEK_END
lseek-->>lseek_main: Move FD to offset from end
else SEEK_TYPE is SEEK_CUR or invalid
lseek-->>lseek_main: Move FD to offset from current
end
else Invalid arguments
lseek_main-->>User: Error message
end
lseek_main-->>User: Return result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @jkool702 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the repeated lseek calls and error handling into a helper function to reduce code duplication and improve maintainability
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| fprintf(stderr, "\nIncorrect number of arguments.\nUSAGE: lseek <FD> <REL_OFFSET>\n"); | ||
| // check for exactly 2 or 3 args passed to lseek | ||
| if (argc != 3 && argc != 4) { | ||
| fprintf(stderr, "\nIncorrect number of arguments.\nUSAGE: lseek <FD> <REL_OFFSET> [<SEEK_TYPE>]\n"); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
suggestion: Consider consolidating the duplicate lseek error handling code
You could move the whence parameter selection before the lseek call and have a single error handling block. This would reduce code duplication and make maintenance easier.
int whence;
if (argc == 4) {
if (strcmp(argv[3], "SEEK_SET") == 0) {
whence = SEEK_SET;
} else if (strcmp(argv[3], "SEEK_END") == 0) {
whence = SEEK_END;
}
}
if (lseek(fd, offset, whence) == (off_t) -1) {
fprintf(stderr, "\nERROR: %s\n", strerror(errno));
return 1;
}| @@ -65,18 +69,30 @@ | |||
| return 1; | |||
| } | |||
|
|
|||
| // get + validate (relative) offset | |||
| // get + validate (relative) offset | |||
| errno = 0; | |||
| off_t offset = atoll(argv[2]); | |||
There was a problem hiding this comment.
suggestion (bug_risk): Add validation for non-numeric offset input
atoll() returns 0 for invalid input without setting errno. Consider using strtoll() instead, which allows for better error detection.
Suggested change
| off_t offset = atoll(argv[2]); | |
| char *endptr; | |
| off_t offset = strtoll(argv[2], &endptr, 10); | |
| if (*endptr != '\0') { | |
| fprintf(stderr, "Invalid offset: not a number\n"); | |
| return 1; | |
| } |
lseek_builtin/lseek.c
Outdated
| if (lseek(fd, offset, SEEK_CUR) == (off_t) -1) { | ||
| fprintf(stderr, "\nERROR: %s\n", strerror(errno)); | ||
| return 1; | ||
| // get SEEK_TYPE and call lseek to move fd byte offset |
There was a problem hiding this comment.
issue (complexity): Consider consolidating the seek type handling and error checking into a single code path.
The error handling code is duplicated three times unnecessarily. Here's how to simplify while maintaining all functionality:
// get SEEK_TYPE and call lseek to move fd byte offset
int whence = SEEK_CUR; // default
if (argc == 4) {
if (strcmp(argv[3], "SEEK_SET") == 0) {
whence = SEEK_SET;
} else if (strcmp(argv[3], "SEEK_END") == 0) {
whence = SEEK_END;
}
}
if (lseek(fd, offset, whence) == (off_t) -1) {
fprintf(stderr, "\nERROR: %s\n", strerror(errno));
return 1;
}This maintains identical behavior while:
- Reducing code duplication
- Making error handling consistent
- Improving maintainability for future seek types
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
add support to the lseek loadable builtin to use SEEK_SET and SEEK_END
Summary by Sourcery
Enhance the lseek loadable builtin by adding support for SEEK_SET and SEEK_END options, updating the usage documentation and error messages accordingly.
New Features:
Enhancements: