megaraid: Properly handle permanent Path and device Path#20
Merged
TeddyAndrieux merged 1 commit intomainfrom May 12, 2025
Merged
megaraid: Properly handle permanent Path and device Path#20TeddyAndrieux merged 1 commit intomainfrom
TeddyAndrieux merged 1 commit intomainfrom
Conversation
The logic has been moved to domain as it shouldn't depends on the implementation even if it's only used for megaraid for now
TeddyAndrieux
force-pushed
the
bugfix/handle-permanent-path
branch
from
090c8cb to
aea8343
Compare
vdaviot
reviewed
May 9, 2025
| // NOTE: For physical drives backed by hardware RAID controllers this might not | ||
| // be available. | ||
| // nolint: funlen,nestif // This function is pretty simple to follow. | ||
| func (pd *PhysicalDrive) ComputePaths() error { |
Contributor
There was a problem hiding this comment.
WDYT about writing this as 2 methods (permanent and device) that returns the computed values instead of modifying implicitly ?
Better error handling, readability and testability IMO
Contributor
Author
There was a problem hiding this comment.
Since it's a method of physicalDrive struct to me it's cleaner to update in place
For separating permanent and device I could but one depends on the other and in all case we want to compute boths so... I don't know if it brings a lot of value proper error message are handled by this function and not the upper ones
Comment on lines
+576
to
+581
| err = pd.ComputePaths() | ||
| if err != nil { | ||
| return devicePath, "", errors.Wrap(err, "failed to compute paths from physical drive") | ||
| } | ||
|
|
||
| return pd.DevicePath, pd.PermanentPath, nil |
Contributor
There was a problem hiding this comment.
Cf com above, would make this part less implicity and enable more precise error handling
vdaviot
approved these changes
May 12, 2025
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The logic has been moved to domain as it shouldn't depends on the implementation even if it's only used for megaraid for now