Fix accepting empty path by diskutil (#330)

* Fix accepting empty path by `diskutil` * Improve json parsing - from_slice instead of from_reader
boozook · Apr 27, 2024 · d1187cb · d1187cb
1 parent 03cc748
commit d1187cb
Show file tree

Hide file tree

Showing 4 changed files with 193 additions and 10 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/support/device/Cargo.toml b/support/device/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "playdate-device"
-version = "0.2.6"
+version = "0.2.7"
 readme = "README.md"
 description = "Cross-platform interface Playdate device, async & blocking."
 keywords = ["playdate", "usb", "serial"]

diff --git a/support/device/src/mount/mac.rs b/support/device/src/mount/mac.rs
@@ -166,8 +166,17 @@ fn spusb<F>(filter: F)
 	let output = Command::new("system_profiler").args(["-json", "SPUSBDataType"])
 	                                            .output()?;
 	output.status.exit_ok()?;
+	parse_spusb(filter, &output.stdout)
+}
+
 
-	let data: SystemProfilerResponse = serde_json::from_reader(&output.stdout[..])?;
+fn parse_spusb<F>(
+	filter: F,
+	data: &[u8])
+	-> Result<impl Iterator<Item = SpusbInfo<impl Future<Output = Result<PathBuf, Error>>>>, Error>
+	where F: FnMut(&DeviceInfo) -> bool
+{
+	let data: SystemProfilerResponse = serde_json::from_slice(data)?;
 
 	let result = data.data
 	                 .into_iter()
@@ -189,7 +198,7 @@ fn spusb<F>(filter: F)
 					                                        Some(futures_lite::future::ready(Ok(path)).left_future())
 				                                        } else {
 					                                        let path = Path::new("/Volumes").join(&par.name);
-					                                        if path.exists() {
+					                                        if !par.name.trim().is_empty() && path.exists() {
 						                                        trace!("existing, by name: {}", path.display());
 						                                        Some(futures_lite::future::ready(Ok(path)).left_future())
 					                                        } else if par.volume_uuid.is_some() {
@@ -218,16 +227,20 @@ async fn mount_point_for_partition(par: MediaPartitionInfo) -> Result<PathBuf, E
 		                                     .arg(volume_uuid)
 		                                     .output()?;
 		output.status.exit_ok()?;
-
-		let info: DiskUtilResponse = plist::from_bytes(output.stdout.as_slice())?;
-		info.mount_point
-		    .ok_or(Error::MountNotFound(format!("{} {}", &par.name, &par.bsd_name)))
-		    .map(PathBuf::from)
+		parse_diskutil_info(&par, output.stdout.as_slice())
 	} else {
 		Err(Error::MountNotFound(format!("{} {}", &par.name, &par.bsd_name)))
 	}
 }
 
+fn parse_diskutil_info(par: &MediaPartitionInfo, data: &[u8]) -> Result<PathBuf, Error> {
+	let info: DiskUtilResponse = plist::from_bytes(data)?;
+	info.mount_point
+	    .filter(|s| !s.trim().is_empty())
+	    .ok_or(Error::MountNotFound(format!("{} {}", par.name, par.bsd_name)))
+	    .map(PathBuf::from)
+}
+
 
 #[derive(Deserialize, Debug)]
 struct DiskUtilResponse {
@@ -275,3 +288,172 @@ pub struct MediaPartitionInfo {
 	volume_uuid: Option<String>,
 	mount_point: Option<PathBuf>,
 }
+
+
+#[cfg(test)]
+mod tests {
+	use std::path::Path;
+
+	use futures::FutureExt;
+	use super::MediaPartitionInfo;
+	use super::parse_spusb;
+	use super::parse_diskutil_info;
+
+
+	#[test]
+	fn parse_spusb_not_mount() {
+		let data = r#"
+		{
+			"SPUSBDataType" : [
+			  {
+				 "_items" : [
+					{
+					  "_name" : "Playdate",
+					  "serial_num" : "PDU1-Y000042",
+					  "vendor_id" : "0x1331"
+					}
+				 ]
+			  }
+			]
+		 }
+		"#;
+
+		let res = parse_spusb(|_| true, data.as_bytes()).unwrap().count();
+		assert_eq!(0, res);
+	}
+
+
+	#[test]
+	fn parse_spusb_mount_complete() {
+		let data = r#"
+		{
+			"SPUSBDataType" : [
+			  {
+				 "_items" : [
+					{
+					  "_name" : "Playdate",
+					  "Media" : [
+						 {
+							"volumes" : [
+							  {
+								 "_name" : "PLAYDATE",
+								 "bsd_name" : "disk9s1",
+								 "mount_point" : "/Volumes/PLAYDATE",
+								 "volume_uuid" : "1AA11111-111A-311A-11A1-1AA111A1A1A1"
+							  }
+							]
+						 }
+					  ],
+					  "serial_num" : "PDU1-Y000042",
+					  "vendor_id" : "0x1331"
+					}
+				 ]
+			  }
+			]
+		 }
+		"#;
+
+		let dev = {
+			let mut devs: Vec<_> = parse_spusb(|_| true, data.as_bytes()).unwrap().collect();
+			assert_eq!(1, devs.len());
+			devs.pop().unwrap()
+		};
+
+		assert_eq!(dev.name, "Playdate");
+		assert_eq!(dev.serial, "PDU1-Y000042");
+
+		let vol = dev.volume.now_or_never().unwrap().unwrap();
+		assert_eq!("/Volumes/PLAYDATE", vol.to_string_lossy());
+	}
+
+
+	#[test]
+	fn parse_spusb_mount_incomplete() {
+		let data = r#"
+		{
+			"SPUSBDataType" : [
+			  {
+				 "_items" : [
+					{
+					  "_name" : "Playdate",
+					  "Media" : [
+						 {
+							"volumes" : [
+							  {
+								 "_name" : "PLAYDATE",
+								 "bsd_name" : "disk9s1",
+								 "file_system" : "MS-DOS FAT32",
+								 "iocontent" : "Windows_FAT_32",
+								 "size" : "3.66 GB",
+								 "size_in_bytes" : 3663724032,
+								 "volume_uuid" : "1AA11111-111A-311A-11A1-1AA111A1A1A1"
+							  }
+							]
+						 }
+					  ],
+					  "serial_num" : "PDU1-Y000042",
+					  "vendor_id" : "0x1331"
+					}
+				 ]
+			  }
+			]
+		 }
+		"#;
+
+		let dev = {
+			let mut devs: Vec<_> = parse_spusb(|_| true, data.as_bytes()).unwrap().collect();
+			assert_eq!(1, devs.len());
+			devs.pop().unwrap()
+		};
+
+		assert_eq!(dev.name, "Playdate");
+		assert_eq!(dev.serial, "PDU1-Y000042");
+
+		let vol = dev.volume.now_or_never();
+		assert!(matches!(vol, Some(Err(_))));
+	}
+
+
+	#[test]
+	fn parse_diskutil_info_complete() {
+		let data = r#"
+		<?xml version="1.0" encoding="UTF-8"?>
+		<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+		<plist version="1.0">
+		<dict>
+			<key>MountPoint</key>
+			<string>/Vols/NAME</string>
+		</dict>
+		</plist>
+		"#;
+
+		let partition = MediaPartitionInfo { name: "name".to_owned(),
+		                                     bsd_name: "bsd_name".to_owned(),
+		                                     volume_uuid: None,
+		                                     mount_point: None };
+		let path = parse_diskutil_info(&partition, data.as_bytes()).unwrap();
+		assert_eq!(Path::new("/Vols/NAME"), path);
+	}
+
+
+	#[test]
+	fn parse_diskutil_info_incomplete() {
+		let data = r#"
+		<?xml version="1.0" encoding="UTF-8"?>
+		<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+		<plist version="1.0">
+		<dict>
+			<key>MountPoint</key>
+			<string></string>
+		</dict>
+		</plist>
+		"#;
+
+		let partition = MediaPartitionInfo { name: "name".to_owned(),
+		                                     bsd_name: "bsd_name".to_owned(),
+		                                     volume_uuid: None,
+		                                     mount_point: None };
+		let res = parse_diskutil_info(&partition, data.as_bytes());
+		assert!(res.is_err())
+	}
+}
diff --git a/support/device/src/mount/methods.rs b/support/device/src/mount/methods.rs
@@ -97,10 +97,11 @@ pub async fn wait_fs_available<T>(mount: &MountedDevice, retry: Retries<T>) -> R
 }
 
 
+/// Double wait time and notify user in between of halfs.
 pub async fn wait_fs_available_with_user<T>(mount: &MountedDevice, retry: Retries<T>) -> Result
 	where T: Clone + std::fmt::Debug + IterTime {
 	match wait_fs_available(mount, retry).await {
-		Ok(_) => todo!(),
+		Ok(_) => (),
 		Err(err) => {
 			error!("{err}");
 			warn!(