Improve partition name extraction logic #381
Conversation
Without this fix, a ISO9660 image directly written to a USB stick is not recognized properly. With this fix, it is. Original comment said `// This is a very crude way of removing the partition number`.
probonopd
changed the title
This allows us to recognize USB sticks that have an ISO9660 image directly written on it (e.g., GhostBSD). Upstream PR: gnustep#381
There was a problem hiding this comment.
Pull request overview
This PR improves the partition name extraction logic in NSWorkspace to properly handle modern Linux block device naming schemes. The previous implementation naively truncated device names to 3 characters, which failed for devices like NVMe drives (nvme0n1p1), MMC/SD cards (mmcblk0p1), and ISO9660 images on USB sticks.
Changes:
- Updated copyright year to 2026
- Replaced crude 3-character truncation with comprehensive device-type-aware partition stripping logic
- Added support for nvme, mmcblk, sd, hd, vd, and xvd device naming patterns with proper partition suffix removal
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /* Extract block device name by removing partition suffix | ||
| * Handle various device naming schemes: | ||
| * sd*, hd* → just remove trailing digits (sda1 → sda) | ||
| * nvme*n*p* → remove trailing p<digits> (nvme0n1p1 → nvme0n1) | ||
| * mmcblk*p* → remove trailing p<digits> (mmcblk0p1 → mmcblk0) | ||
| * loop*, ram*, sr* → use as-is | ||
| */ | ||
| if ([devName hasPrefix:@"nvme"] || [devName hasPrefix:@"mmcblk"]) | ||
| { | ||
| /* For nvme and mmcblk devices, partition names have a 'p' separator | ||
| * e.g., nvme0n1p1, mmcblk0p1 */ | ||
| NSRange pRange = [devName rangeOfString:@"p" options:NSBackwardsSearch]; | ||
| if (pRange.location != NSNotFound) | ||
| { | ||
| /* Check if everything after 'p' is digits */ | ||
| NSString *suffix = [devName substringFromIndex:pRange.location + 1]; | ||
| NSCharacterSet *nonDigits = [[NSCharacterSet decimalDigitCharacterSet] invertedSet]; | ||
| if ([suffix rangeOfCharacterFromSet:nonDigits].location == NSNotFound && [suffix length] > 0) | ||
| { | ||
| /* It's a partition, strip it */ | ||
| devName = [devName substringToIndex:pRange.location]; | ||
| } | ||
| } | ||
| } | ||
| else if ([devName hasPrefix:@"sd"] || [devName hasPrefix:@"hd"] || | ||
| [devName hasPrefix:@"vd"] || [devName hasPrefix:@"xvd"]) | ||
| { | ||
| /* For sd/hd/vd devices, remove trailing digits (sda1 → sda) */ | ||
| NSCharacterSet *digits = [NSCharacterSet decimalDigitCharacterSet]; | ||
| NSInteger i = [devName length] - 1; | ||
| while (i >= 0 && [digits characterIsMember:[devName characterAtIndex:i]]) | ||
| { | ||
| i--; | ||
| } | ||
| if (i < (NSInteger)[devName length] - 1) | ||
| { | ||
| /* Found trailing digits, strip them */ | ||
| devName = [devName substringToIndex:i + 1]; | ||
| } | ||
| } | ||
| /* For other devices (loop, ram, sr, etc.), use as-is */ |
There was a problem hiding this comment.
The new partition name extraction logic handles various device naming schemes but lacks test coverage. Consider adding unit tests that verify correct behavior for different device name patterns such as: sda1→sda, nvme0n1p1→nvme0n1, mmcblk0p1→mmcblk0, sr0→sr0 (unchanged), and edge cases like devices without partition numbers.
| else if ([devName hasPrefix:@"sd"] || [devName hasPrefix:@"hd"] || | ||
| [devName hasPrefix:@"vd"] || [devName hasPrefix:@"xvd"]) | ||
| { | ||
| /* For sd/hd/vd devices, remove trailing digits (sda1 → sda) */ |
There was a problem hiding this comment.
The comment mentions only "sd/hd/vd devices" but the code also handles "xvd" devices (Xen virtual disks). The comment should include "xvd" for completeness.
| /* For sd/hd/vd devices, remove trailing digits (sda1 → sda) */ | |
| /* For sd/hd/vd/xvd devices, remove trailing digits (sda1 → sda) */ |
| // This is a very crude way of removing the partition number | ||
| if ([devName length] > 3) | ||
| devName = [devName substringToIndex: 3]; | ||
|
|
There was a problem hiding this comment.
This line contains trailing whitespace which should be removed to maintain code cleanliness.
|
|
||
| /* Extract block device name by removing partition suffix | ||
| * Handle various device naming schemes: | ||
| * sd*, hd* → just remove trailing digits (sda1 → sda) |
There was a problem hiding this comment.
The comment states that only sd* and hd* devices have trailing digits removed, but the code also handles vd* and xvd* devices (virtual disk devices). The comment should be updated to reflect all device types handled: "sd*, hd*, vd*, xvd* → remove trailing digits".
| * sd*, hd* → just remove trailing digits (sda1 → sda) | ||
| * nvme*n*p* → remove trailing p<digits> (nvme0n1p1 → nvme0n1) | ||
| * mmcblk*p* → remove trailing p<digits> (mmcblk0p1 → mmcblk0) | ||
| * loop*, ram*, sr* → use as-is |
There was a problem hiding this comment.
This line should be removed as it gets repeated on line 1331.
fredkiefer
left a comment
fredkiefer
left a comment
There was a problem hiding this comment.
Looks good to me.
The Copilot comments are really very helpful, your should address these. Having a test would be nice, but not strictly required.
I would prefer to have this new code in a helper function/method, but again this is not required. I also prefer to have a space after the colon in a method call, but the code around this change is also lacking that.
|
@copilot open a new pull request to apply changes based on the comments in this thread (Note: This doesn't seem to work; apparently the repo is not configured to allow it. So I will work on it manually, although I think Copilot would have been capable to do this sort of thing.) |
2 participants
Without this fix, a ISO9660 image directly written to a USB stick (e.g., GhostBSD, Linux Live system) is not recognized properly. With this fix, it is.
Original comment said
// This is a very crude way of removing the partition number.