WIP expose disk firmware in inventory #5706
Conversation
| pub struct DiskFirmware { | ||
| active_slot: u8, | ||
| next_active_slot: Option<u8>, | ||
| slot1_read_only: bool, |
There was a problem hiding this comment.
It feels odd that all the other fields imply an arbitrary number of slots, but this field specifically embeds the notion of "slot1"? Also, is this one or zero-indexed?
There was a problem hiding this comment.
Welcome to the NVMe spec! The spec states that devices have slot 1-7 (and potentially more in the future as there's unused padding at the end of the C struct). The spec also says that only slot 1 can be read only. Not every device will have all 7 slots available, for example the C bhyve nvme devices shows only one slot that is read only, while some of our WDC devices show 4 slots. And not every available slot will necessarily contain firmware. The firmware version itself is defined to be an ascii string hence the Vec<Option<String>>.
After having chatted with @jgallagher about this we landed on NvmeSlot being a wrapping type to address the array index being 0 but corresponding with slot1 - this can be seen here and has yet to be officially reviewed. The thought was that one could gain access to all the slots via an iterator and upstream of the library you could deal with the 0 indexed array.
I originally had this represented as an enum but was unsure what the underlying CRDB type should look like. Very open to suggestions here as I just went with something for this PR as a placeholder.
| active_slot: u8, | ||
| next_active_slot: Option<u8>, | ||
| slot1_read_only: bool, | ||
| slots: Vec<Option<String>>, |
There was a problem hiding this comment.
What's the value of this String? This feels a little bit like a more specific type that has been coerced into a string?
There was a problem hiding this comment.
See the above comment.
| let mut removed = Vec::new(); | ||
| let mut updated = Vec::new(); | ||
|
|
||
| // Find new or udpated disks. |
There was a problem hiding this comment.
| // Find new or udpated disks. | |
| // Find new or updated disks. |
| let controller_lock = match controller.try_read_lock() { | ||
| libnvme::controller::TryLockResult::Ok(locked) => locked, | ||
| // We should only hit this if something in the system has locked the | ||
| // controller in question for writing. | ||
| libnvme::controller::TryLockResult::Locked(_) => { | ||
| warn!( | ||
| log, | ||
| "NVMe Controller is already locked so we will try again | ||
| in the next hardware snapshot" | ||
| ); | ||
| return Err(Error::NvmeControllerLocked); | ||
| } | ||
| libnvme::controller::TryLockResult::Err(err) => { | ||
| return Err(Error::from(err)) | ||
| } | ||
| }; |
There was a problem hiding this comment.
👍 this makes sense, given our prior conversation
| // Today the only update we expect to be able to apply to | ||
| // a `Disk` is firmware. | ||
| pub(crate) fn update_disk(&mut self, raw_disk: &RawDisk) { |
There was a problem hiding this comment.
I wonder if we could program this a little bit more defensively, in case there are other parts of these structures that can be updated. The comment indicates the true behavior ("we ignore everything but the firmware") but the calllsites don't really make that clear.
Can we either call this update_firmware, or have some other way to make sure "we don't drop updates on the floor" if something other than the firmware field changes?
There was a problem hiding this comment.
I can fix this - I had originally left this as update_disk as there might be more fields in the future we wish to support. But I am happy to make this an explicit update_firmware for the time being. (side note... maybe update_firmware_metadata is a better name).
papertigers
force-pushed
the
mike/nvme-firmware-inventory
branch
from
6bc3a9c
to
1ee77ff
Compare
papertigers
force-pushed
the
mike/nvme-firmware-inventory
branch
from
46347bf
to
fa35f33
Compare
No description provided.