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..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"]:

Ah I didn't realize `all([]) = True`. Hmm I think maybe checking for the length explicitly and doing an early return would solve it too without having to use `any`?

```
partitions = ptable.get("partitions", [])
if len(partitions) == 0:
  return False

return all(part.get("type") == "42" for part in partitions)
```

I found the `all` way to be slightly simpler to read but I'm okay with the current implementation for sure. I meant the suggestion to be non-blocking - sorry for not making that clear - so feel free to take it or leave it.

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


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