curtin-dev team mailing list archive

[Chris Peterson <mp+480694@xxxxxxxxxxxxxxxxxx>]

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.

typo: s/will/with

> +
> +            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):

question: It's not clear to me what this test is testing. I'm inferring from the name that we want to assert /dev/sda5 is not in /dev/sda's partition table (my understanding is the last partition is the ldm partition which contains real information about the others?) but here we just assert some information about just sda5 itself. Maybe I just don't understand fully though. Would you mind elaborating please?

> +        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.