[intel] overlapping partitions cannot be detected

[rainbow-demon]

Haiku hrev38293 doesn't see partitions on my hard drive, though Windows XP sees all of them.

diskpart output from Windows XP:

DISKPART> 
  Partition ###  Type           Size   Offset
  -------------  ----------------  -------  -------
  Partition 1    Primary            80 GB    32 KB
  Partition 2    Primary            66 GB    80 GB
  Partition 3    OEM               2778 MB   146 GB
DISKPART>

fdisk -l output from Ubuntu

Disk /dev/sda: 160.0 GB, 160041885696 bytes
255 heads, 63 sectors/track, 19457 cylinders
Units = cylinders of 16065 * 512 = 8225280 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disk identifier: 0x73657365

   Device Boot      Start         End      Blocks   Id  System
/dev/sda1   *           1       10444    83891398+   7  HPFS/NTFS
/dev/sda2           10445       19104    69561450    b  W95 FAT32
/dev/sda4           19104       19458     2844163   de  Dell Utility 

You can see that sda2 and sda4 are overlapped just by one sector and no partitions at all are detected.

The most interesting line in syslog is 2295, just to be sure.

[korli]

Just to be sure, to which partition is the common sector assigned in your case (ie on Linux) ?

Can you actually mount sda2 and sda4 on Linux ?

[rainbow-demon]

Replying to korli:

Just to be sure, to which partition is the common sector assigned in your case (ie on Linux) ?

Can you actually mount sda2 and sda4 on Linux ?

Common sector actually belongs to sda4; sda2 filesystem is shorter than sda2 partition, so it can be mounted without real overlapping.

Yes, ubuntu can do it with some magic behind the scenes, but mount told me about partitioning errors. gparted agrees with Haiku and refuses to detect partitions.

sda4 is a hidden partition to restore Windows XP preinstalled by OEM supplier.

[stippi]

I remember seeing some code which does a plausibility test and refuses to accept the partition layout in Haiku, for this above case exactly. I don't see how Haiku could properly check this case, since partition layout scanning happens independent of contained file systems, so the partition scanning would not know that the file system contained in one of the overlapping partitions has an internal size that removes the overlap. As soon as you initialize this partition with another file system, you'd have a problem. Invalid partition maps are just bad. What should happen if you have a partition that completely lies within another partition?

[korli]

Replying to stippi:

I remember seeing some code which does a plausibility test and refuses to accept the partition layout in Haiku, for this above case exactly. I don't see how Haiku could properly check this case, since partition layout scanning happens independent of contained file systems, so the partition scanning would not know that the file system contained in one of the overlapping partitions has an internal size that removes the overlap. As soon as you initialize this partition with another file system, you'd have a problem. Invalid partition maps are just bad. What should happen if you have a partition that completely lies within another partition?

It happens ​here.

The case of rainbow-demon seems to be a corner case. I would suggest to check if the overlap is exactly one sector and then decrease the size of the partition object.

[rainbow-demon]

Replying to stippi:

What should happen if you have a partition that completely lies within another partition?

File system header check. If disk partitions look like matryoshka -- [ sda1 [ sda2 ] ] (very unlikely), and sda1's file system is bigger than len(sda1)-len(sda2) -- it is definitely a serious error and those partitions cannot be mounted. If not (file systems have no common sectors), there should be an error message. We can mount them -- but without pleasure.

Hmm, I just noticed a mistake - partitions overlap by one block, not by one sector.

[stippi]

I've tried to explain this in my previous comment, detecting the file systems and using their information when parsing partitions is a layering violation. The (right now) clean design of the Haiku Disk Device API would have to be completely changed just to gracefully handle a situation that shouldn't happen in the first place, and which can be handled gracefully only in corner cases. Where do you stop? Suppose we introduce the layering violation to detect and allow your situation, then the next user comes along and says: Well, I have overlapping partitions, and even my file systems actually overlap according to the allocated size of each, but -- by pure chance -- I don't have any data in the region where they overlap, can you please allow this? :-)

[rainbow-demon]

Replying to stippi:

I've tried to explain this in my previous comment, detecting the file systems and using their information when parsing partitions is a layering violation. The (right now) clean design of the Haiku Disk Device API would have to be completely changed just to gracefully handle a situation that shouldn't happen in the first place, and which can be handled gracefully only in corner cases. Where do you stop? Suppose we introduce the layering violation to detect and allow your situation, then the next user comes along and says: Well, I have overlapping partitions, and even my file systems actually overlap according to the allocated size of each, but -- by pure chance -- I don't have any data in the region where they overlap, can you please allow this? :-)

I'm sorry :-) maybe overlapping can be simply ignored on this stage, leaving the final decision about accessing those partitions up to user (me ;-) ), warning him about a chance of fatal consequences for his data? Or at least show something in drivesetup - not just like my entire hard disk is empty and all data is lost (this scared me).

Windows can read the partition table and mount sda1 and sda2 (sda4 has ext2 and cannot be mounted though it is seen in Disk Manager).

[axeld]

This is a classical problem, and usually I think one should always try to be as forgiving as possible when reading data. Ie. always write standards conforming data, but do an effort and try to read anything out there.

We could just cut off the overlapping part of the earlier partition always, and let the file system decide whether or not it fits in there - BFS already checks this during mount. If the file system doesn't fit, it could mount itself read-only at least, so that you could still access the data. That would completely prevent a layering violation, and is still as convenient as it can get without risking any data loss.

[bebop]

Hey guys, just wanted to add a patch that allows for partitions to overlap. This implements the behavior that axel describes, as I understood him.

[korli]

Replying to bebop:

Hey guys, just wanted to add a patch that allows for partitions to overlap. This implements the behavior that axel describes, as I understood him.

Nice! Though you should check (i > 0) before accessing the array. Please also check the coding style: there spaces between i, minus and 1.

const Partition* previousPartition = byOffset[i - 1];

The minus operator goes on the next line

off_t previousSize = previousPartition->Size()
- (nextOffset - partition->Offset());  

[bebop]

Thanks korli! the style violations should be fixed.

[stippi]

Patch looks perfect to me. Bebop, don't you have SVN access? If not, I can commit the patch for you.

[bebop]

I do have accesses, however I have not given my ssh key to anyone. Is oliver still in charge of that? Stippi maybe you could just apply this until I get my access figured out.

Thanks

[korli]

Looks OK. I'm just wondering if there should be a limit to what we cut off. Also it might be a good idea to switch the cut off volume to read only.

[bebop]

My biggest problem is that the partition layout in this case is invalid. I almost feel like not parsing the partition layout in this case is the correct thing to do, I also realize that it probably freaks people out to not see any of there partitions. While I like the idea that there is a limit, how far do we go? And limiting the size of the cut seems rather arbitrary. I think that adding a read-only flag to the cut off volume is a great idea, I am not sure, however, if partitions have a notion of read only though. Maybe Stippi or bonefish can recall off the top of their head. Otherwise I will investigate tonight and hopefully come up with something suitable.

[stippi]

I just had the idea that the patch does not check the new size not to be negative (the current loop partition starts before the previous partition). That should be done as well. It sounds good to me to defer to filesystems to match the content size with the partition size they are provided.

[bebop]

Good call, I thought about that at one point, but apparently it did not stick in my head :) Do you remember if a partition has a read only flag? and if DriveSetup (or the api for that matter) respects that?

[bonefish]

Replying to bebop:

Hey, welcome back, Bryce! :-)

I do have accesses, however I have not given my ssh key to anyone. Is oliver still in charge of that?

Oliver is still the chief sysadmin. Matt, Niels, Axel, and myself can set things up as well. The fastest way is probably to find someone on IRC. Or feel free to send me your key -- I don't have much time ATM, so it may take a bit.

Replying to stippi:

I just had the idea that the patch does not check the new size not to be negative (the current loop partition starts before the previous partition).

This cannot happen, since the partitions are sorted by offset (hence the array name :-)). A check for previousSize == 0 is in order, though.

[axeld]

Replying to bebop:

I think that adding a read-only flag to the cut off volume is a great idea, ...

I don't think this should be done on partition level; it's the job of the file system in question to check if its contents fit on the partition, and react on that. BFS certainly does so already. IOW the file system should decide whether or not becoming read-only is needed.

[bebop]

Ok, so if the offsets are the same => previousSize == 0, is covered, the base case is covered, as stated above a sort is performed on the offsets, so the offsets are either equal or the current offset is strictly greater than the previous offset.

Let the file systems take care of the partition being to short. I think that this takes care of all the cases. Thanks for all the feed back. Good to be writing code for Haiku again :-)

[bonefish]

Replying to bebop:

Ok, so if the offsets are the same => previousSize == 0, is covered, the base case is covered, as stated above a sort is performed on the offsets, so the offsets are either equal or the current offset is strictly greater than the previous offset.

Let the file systems take care of the partition being to short.

Well, a file system can't do much about a too short partition. Access via a file descriptor to the partition is restricted to the partition's bounds. The only thing the file system could to is to open the parent partition or device, but that is strongly discouraged.