src/* : use lsblk --json output format #631
Conversation
sohankunkerkar
force-pushed
the
lsblk-change
branch
from
8d2b40d
to
6901144
Compare
src/bin/rdcore/rootmap.rs
Outdated
| let devinfo = Device::lsblk(Path::new(devpath), false)?; | ||
| let fs = devinfo[0] | ||
| .fstype | ||
| .as_ref() |
There was a problem hiding this comment.
We should be able to drop the as_ref() and just consume the devinfo. But we need to change the Device::lsblk API first (see below).
src/blockdev.rs
Outdated
| .uuid | ||
| .as_ref() | ||
| .ok_or_else(|| anyhow::anyhow!("missing uuid in {}", devinfo[0].name))?; | ||
| Ok(uuid.to_string()) |
There was a problem hiding this comment.
This is a good example where consuming would be more efficient because we don't have to clone the string again.
src/blockdev.rs
Outdated
| } | ||
|
|
||
| impl Device { | ||
| pub fn lsblk(dev: &Path, with_deps: bool) -> Result<Vec<Device>> { |
There was a problem hiding this comment.
So I think here a really nice thing about switching to JSON is that we capture the hierarchy of the block devices. In this case, subdevices of the main device we're querying will be in children.
So here I think we want to output a Result<Device> instead. The lsblk output supports outputting multiple top-level devices, but in our uses of lsblk we're always really just interested in a single top-level device (and possibly its children if with_deps is enabled). So this function could sanity-check that only one device matched, and return that one.
There was a problem hiding this comment.
I think we should also rename with_deps to with_children, since that's the name in the structure we return.
Though hmm, kinda tempting to drop the with_deps argument altogether since we'd always return one device now unlike before. lsblk will be doing more work than it needs to in some cases, but meh... feels like it's worth the cleaner API we get. @bgilbert might have a different opinion.
src/blockdev.rs
Outdated
| let deviceinfos: Vec<Device> = Device::lsblk(Path::new(&self.path), true)?; | ||
| for deviceinfo in deviceinfos { | ||
| let mut partition = self.compute_partition(&deviceinfo, with_holders)?; | ||
| result.append(&mut partition); | ||
| if let Some(children) = deviceinfo.children.as_ref() { | ||
| if !children.is_empty() { | ||
| for child in children { | ||
| let mut childpartitions = self.compute_partition(child, with_holders)?; | ||
| result.append(&mut childpartitions) | ||
| } | ||
| } |
There was a problem hiding this comment.
Hmm, right OK you had to do some refactoring here to deal with children not being in the main iteration anymore.
Though I don't think this flow is functionally equivalent, because we're only going one level down. I think we want a recursive model here instead? So e.g. you have get_partitions call to get_partitions_recurse with the top-level Device, and then that function calls itself for each child in the device, aggregating partitions in the vector as it goes. Then we return that vector. Does that make sense?
sohankunkerkar
force-pushed
the
lsblk-change
branch
2 times, most recently
from
a05ed4f
to
90fccd4
Compare
This is almost an exact revert of coreos#308, which was necessary at the time when RHCOS shipped with its rootfs inside a LUKS container. It no longer does that, so let's simplify the code.
| let devinfo = devs | ||
| .blockdevices | ||
| .into_iter() | ||
| .next() | ||
| .ok_or_else(|| anyhow!("failed to get device information"))?; |
There was a problem hiding this comment.
This should verify that there is only one dev and return that dev. Otherwise error out.
src/blockdev.rs
Outdated
| .uuid | ||
| .as_ref() | ||
| .ok_or_else(|| anyhow::anyhow!("missing uuid in {}", devinfo[0].name))?; | ||
| Ok(uuid.to_string()) |
src/blockdev.rs
Outdated
| let deviceinfo = Device::lsblk(Path::new(&self.path), true)?; | ||
| let mut partition = self.compute_partition(&deviceinfo, with_holders)?; | ||
| if !partition.is_empty() { | ||
| result.append(&mut partition); | ||
| } | ||
| let mut childpartitioninfo = self.get_partitions_recurse(&deviceinfo, with_holders)?; | ||
| if !childpartitioninfo.is_empty() { | ||
| result.append(&mut childpartitioninfo); | ||
| } | ||
| Ok(result) | ||
| } | ||
|
|
||
| fn get_partitions_recurse( | ||
| &self, | ||
| deviceinfo: &Device, | ||
| with_holders: bool, | ||
| ) -> Result<Vec<Partition>> { | ||
| let mut result: Vec<Partition> = Vec::new(); | ||
| if let Some(children) = deviceinfo.children.as_ref() { | ||
| if !children.is_empty() { | ||
| for child in children { | ||
| let mut childpartitions = self.compute_partition(child, with_holders)?; | ||
| result.append(&mut childpartitions); | ||
| self.get_partitions_recurse(child, with_holders)?; | ||
| } |
There was a problem hiding this comment.
OK digging more into this took me to #648. Let's do that first, and then this should essentially be just get_partitions() enumerating over the immediate children only for partitions (so no need for recursion).
Also, maybe as a follow-up we should investigate just returning Vec<Device> here instead of a separate Partition type.
sohankunkerkar
force-pushed
the
lsblk-change
branch
from
90fccd4
to
cf2bfcc
Compare
Fixes: #516