Diff comments:

> diff --git a/curtin/storage_config.py b/curtin/storage_config.py
> index dae89f4..6e28187 100644
> --- a/curtin/storage_config.py
> +++ b/curtin/storage_config.py
> @@ -470,6 +470,39 @@ class ProbertParser(object):
>          return multipath.is_mpath_partition(
>              blockdev.get('DEVNAME', ''), blockdev)
>  
> +    @staticmethod
> +    def looks_like_ldm_disk(blockdev) -> bool:
> +        """ Tell if the disk looks like a Windows dynamic disk or LDM (aka.
> +            Logical Disk Manager).
> +
> +            Here we consider that a disk is a dynamic disk if it contains a 
> DOS
> +            partition table will all partitions having type 0x42.

Thanks!

> +
> +            The Linux kernel and possibly libldm (currently in universe) do
> +            extra checks to determine if a disk is a dynamic disk.
> +
> +            Here we only scratch the surface (thus the verb "looks_like" 
> rather
> +            than "is") so it is better to only use this function if we 
> already
> +            suspect the disk could be a LDM disk.
> +        """
> +        found_one = False
> +        try:
> +            ptable = blockdev["partitiontable"]
> +            # Currently, the Linux kernel only supports dynamic disks on dos
> +            # partition tables.
> +            if ptable["label"] != "dos":
> +                return False
> +            for part in ptable["partitions"]:
> +                # This used to be "SFS" but it is ancient and Windows dynamic
> +                # disks use it too.
> +                if part.get("type") != "42":
> +                    return False
> +                found_one = True
> +        except KeyError:
> +            return False
> +
> +        return found_one
> +
>      def blockdev_to_id(self, blockdev):
>          """ Examine a blockdev dictionary and return a tuple of curtin
>              storage type and name that can be used as a value for
> diff --git a/tests/unittests/test_storage_config.py 
> b/tests/unittests/test_storage_config.py
> index 7b0f68c..fc4b1e1 100644
> --- a/tests/unittests/test_storage_config.py
> +++ b/tests/unittests/test_storage_config.py
> @@ -531,6 +598,35 @@ class TestBlockdevParser(CiTestCase):
>              self.assertDictEqual(expected_dict,
>                                   self.bdevp.asdict(blockdev))
>  
> +    def test_blockdev_asdict_ldm_disk(self):
> +        self.probe_data = _get_data('probert_storage_ldm.json')
> +        self.bdevp = BlockdevParser(self.probe_data)
> +        blockdev = self.bdevp.blockdev_data['/dev/sda']
> +        expected_dict = {
> +            'id': 'disk-sda',
> +            'type': 'disk',
> +            'wwn': '0x5000039fdcf04730',
> +            'serial': 'TOSHIBA_HDWD110_999E6AZFS',
> +            'ptable': 'unsupported',
> +            'path': '/dev/sda',
> +        }
> +        self.assertDictEqual(expected_dict, self.bdevp.asdict(blockdev))
> +
> +    def test_blockdev_asdict_ldm_partition_not_in_ptable(self):

Sure, I'll clarify in the code. Essentially, this code used to raise "Partition 
entry not found in table". I'm asserting that we now get valid data, even 
though there's no mention of sda5 in the ptable.

> the last partition is the ldm partition which contains real information about 
> the others

Not quite, I don't think there is such as thing as a "LDM partition". There is 
a LDM database (which is sort-of an extra partition table) in the last MiB of 
the disk. 

If my understanding is correct, LDM uses a MSDOS ptable (GPT is also a thing 
but..) with partition(s) to make the disk "look" full. This way, if we don't 
realize it's a "dynamic" disk, we are not tempted to use any "free space" - 
which wouldn't actually be free space.

Now, I don't really understand the difference between a dynamic disk where the 
ptable shows:
* a single partition spanning the whole disk
* multiple partitions (covering the whole disk)

Maybe it depends how many partitions existed before it got converted to a 
dynamic disk/LDM. I'm just guessing here.

> +        self.probe_data = _get_data('probert_storage_ldm.json')
> +        self.bdevp = BlockdevParser(self.probe_data)
> +        blockdev = self.bdevp.blockdev_data['/dev/sda5']
> +        expected_dict = {
> +            'id': 'partition-sda5',
> +            'type': 'partition',
> +            'device': 'disk-sda',
> +            'path': '/dev/sda5',
> +            'number': 5,
> +            'offset': 904945664 * 512,
> +            'size': 536869863424,
> +        }
> +        self.assertDictEqual(expected_dict, self.bdevp.asdict(blockdev))
> +
>      def test_blockdev_multipath_disk(self):
>          self.probe_data = _get_data('probert_storage_multipath.json')
>          self.bdevp = BlockdevParser(self.probe_data)


-- 
https://code.launchpad.net/~ogayot/curtin/+git/curtin/+merge/480694
Your team curtin developers is requested to review the proposed merge of 
~ogayot/curtin:ldm into curtin:master.


-- 
Mailing list: https://launchpad.net/~curtin-dev
Post to     : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help   : https://help.launchpad.net/ListHelp

Reply via email to