curtin-dev team mailing list archive

[Olivier Gayot <mp+480694@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/curtin/storage_config.py b/curtin/storage_config.py
> index dae89f4..2ed4d63 100644
> --- a/curtin/storage_config.py
> +++ b/curtin/storage_config.py
> @@ -470,6 +470,37 @@ class ProbertParser(object):
>          return multipath.is_mpath_partition(
>              blockdev.get('DEVNAME', ''), blockdev)
>  
> +    def looks_like_ldm_disk(self, 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.
> +
> +            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"]:

We would need an extra check or it would return True for a disk with 0 partitions. 

all([]) => True

```
x = [part.get("type") == "42" for part in ptable["partitions"]]

return any(x) and all(x)  # make sure at least one value is True first
return x and all(x)       # or use implicit bool conversion
```

 would work though. Do you prefer this over the current implementation?

> +                # This used to be "SFS" 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
> @@ -797,8 +831,20 @@ class BlockdevParser(ProbertParser):
>                          break
>  
>                  if part is None:
> -                    raise RuntimeError(
> -                        "Couldn't find partition entry in table")
> +                    # Could not find the partition in the partition table.
> +
> +                    # It is expected for LDM (or Windows dynamic disks).
> +                    # The Linux kernel can discover partitions from the "LDM
> +                    # database" which is stored in the last 1MiB of the disk.
> +                    if self.looks_like_ldm_disk(parent_blockdev):
> +                        part = {
> +                            'start': attrs['start'],
> +                            'size': attrs['size'],
> +                            'on-dynamic-disk': True,

I'm sure there will subtleties with almost every type of "unsupported" disk layouts. And I've only run into this one (LDM) so far so I'm not super comfortable trying to handle all ramifications of "unsupported" for now.

But sure, the main problem I'm trying to solve with LDM is that the system sees partitions that are not visible when looking at the ptable.

So I'll try `"visible-in-ptable": False`, if that works for you. We'll see if other types can reuse the same concept.

> +                        }
> +                    else:
> +                        raise RuntimeError(
> +                            "Couldn't find partition entry in table")
>              else:
>                  part = attrs
>  


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