Feat partitionless disks

[taigrr]

This is a continuation of #293 . I've merged in the upstream changes as well.

[taigrr]

@fromanirh would you mind taking a look at this? This is me revisiting #293 . I've added in the unit tests as we discussed. I felt it was a better route than adding a new snapshot once I got a better feel for the code.

[Itxaka]

wont this change the current behavior? Like if I run this against a disk with no partitions and then iterate over the partitions I expect the partitions to be empty, not to return a non-existing partition based on the disk.

Tested locally with an empty dir, main branch returns the disk with no partitions. This branch returns an extra partition. This breaks anything that iterates over partition and expects an empty disk to not have partitions.

For a glaring example see what partitions my BDROM has:

Disk: sr0 ODD (1024MB) SCSI [@pci-0000:00:17.0-ata-2.0 (node #0)] vendor=HL-DT-ST model=HL-DT-ST_BD-RE_BH40N serial=SIK9PH2EE551 removable=true
Partition: sr0 (1024MB) 

I think we should check other params before adding the partition, I understand that the problem this tries to fix is a disk that has a partition on the block device directly, but maybe before adding the partition we should do some sane checks, i.e. does the partition has a FS type on it, or a mountpoint, before adding it to the partition list as to not break backwards compatibility.

[ffromani]

@Itxaka that's a very good point, thanks for raising.
I'll revisit again shortly. One of the problems I have around this area is I'm not really sure the current behaviour is correct, I'll need to review the issue from the beginning.
However, I still believe we will need some change in this area.

[Itxaka]

I am thinking that maybe we can use something similar to #305 in order to try to get the disk FS from udev directly? and if we identify that the disk has a FS type, then maybe we are dealing with a full block device "partition". In my local test with a fully partitioned, not mounted device block I get the following from udev that can help identifying:

E:ID_FS_TYPE=ext4
E:ID_FS_USAGE=filesystem

while in a "normal" partitioned disk, the udev info for the whole devices is as follows:

E:ID_FS_TYPE=

So we could probably use that to identify if the disk contains a full partition :)

[taigrr]

@Itxaka I would need to think more about the suggestion first, but I do like the idea of checking for an FS type. Mountpoints--not so much, just because a disk doesn't have mountpoints doesn't mean we should ignore it. A headless / desktopless system with a portable USB hdd plugged in but not mounted would probably get the wrong answer. I think I'll change this PR to a working draft in the meantime, and I'll pick it back up after #305 is addressed.

[ffromani]

@taigrr thanks! I'll review the issue from the ground up trying to cover all the angles and will share my thoughts (here or in a GH issue). I want to save everyone's time and make sure we reach a solution good for everyone.
I'll get back ASAP, hopefully later this week.

[taigrr]

Hey, so checking back in on this. #305 is now resolved. @fromanirh did you have a chance to revisit this issue? Thanks!

[ffromani]

from my POV this change addresses a real issue and I can't think of a better way to solve this problem. Besides some bookkeeping (resolve conflict, remove merge commit) , LGTM - but I'll have the final review later.
Since this is ultimately a (IMO good) API change, let's see if @jaypipes has any concern.

[jaypipes]

Hey, so checking back in on this. #305 is now resolved. @fromanirh did you have a chance to revisit this issue? Thanks!

@taigrr yes, please rebase and un-draft your PR :) We will review shortly.

[taigrr]

Ok, thanks. Ready for review.

[ffromani]

thanks @taigrr . I want to check we're addressing @Itxaka 's concern raised in #308 (comment) . I'll work a bit more on some test cases that should cover this flow, I'll suggest them in the next review.

[Itxaka]

This still has the same issue with reporting partitions for the wrong devices, i.e. BD-Rom device with no disc on it, still reports a 1024Mb partition.

[ffromani]

thanks @taigrr for your patience and thanks to @Itxaka for the feedback.
I've been thinking on/off about the best way to meet all the requirements here and I can't find a better way than adding a new API. I'll elaborate more in the near future, so we can evaluate if this can make everyone reasonnably happy.

[taigrr]

hey @ffromani did that new api get added? Should I close this PR out?