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

libstoragemgmt: Add support for retrieving hwraid mode from storage t… #52

Closed
wants to merge 1 commit into from

Conversation

joehandzik
Copy link
Member

…argets

Signed-Off-By: Joe Handzik joseph.t.handzik@hpe.com

…argets

Signed-Off-By: Joe Handzik <joseph.t.handzik@hpe.com>
@joehandzik
Copy link
Member Author

We'll see if we hit the same failures. Again, please let me know if there's something structurally wrong with my PRs. My cli code looks...slightly different from yours. But when I try to conform to the model in the rest of the code, I get runtime errors. So let me know if my cli code as it stands is inadequate in particular.

@cathay4t
Copy link
Contributor

Please try to use enum lsm_system_hwraid_mode_type instead of uint32_t *hwraid_mode.

@cathay4t
Copy link
Contributor

Please be informed, once #51 merged,
this pull request needs the rebase.

@joehandzik
Copy link
Member Author

Yup, definitely (re: rebase). Thanks for documenting!

@joehandzik
Copy link
Member Author

Hmm, started looking into the enum usage, and I can't say that I agree. If this was a greenfield class I'd implement an enum, but since I put the system mode inside the System python class, I need to make sure the values I have there don't conflict with the corresponding C values for system statuses (see: STATUS_UNKNOWN, STATUS_OK, etc). I then defined values for LSM_SYSTEM_HWRAID states that wouldn't conflict with the other LSM_SYSTEM values. If I create an enum, I wouldn't necessarily be protected from accidentally setting a future HWRAID value equal to an existing LSM_SYSTEM value.

tldr; I just emulated what already exists for LSM_SYSTEM #defines, and I think it would be more confusing to implement some LSM_SYSTEM values differently from the way existing LSM_SYSTEM values are defined. Is that assertion reasonable, or do you still want me to aggregate LSM_SYSTEM_HWRAID values into an enum?

@cathay4t
Copy link
Contributor

cathay4t commented Oct 1, 2015

@joehandzik To avoid overlap, I am working on this pull request:

  1. Use similar design in pull request New API for retrieving system firmware version. #56 .
  2. Change it to enum lsm_system_hwraid_mode_type to enum lsm_system_mode_type
    which could expand to include 'SAN, NAS, SAN&NAS, OBJ and etc` values in the future.
  3. Add MegaRAID support.

@joehandzik
Copy link
Member Author

Sounds good, sorry for the delay @cathay4t.

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.

None yet

2 participants