curtin-dev team mailing list archive

[Dan Bungert <mp+480694@xxxxxxxxxxxxxxxxxx>]

This looks ok but I'm a bit worried about the BlockdevParser changes, maybe I just don't understand them fully.  

Tests please.

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
> @@ -759,6 +790,9 @@ class BlockdevParser(ProbertParser):
>                  else:
>                      entry['ptable'] = schemas._ptable_unsupported
>  
> +            if self.looks_like_ldm_disk(blockdev_data):
> +                entry['ptable'] = schemas._ptable_unsupported

Let's log this scenario.  I think the heuristic is OK but it's good to have an explanation for the reason.  I feel comfortable with this based on this explanation of the other 0x42 entry SFS:
> SFS is an encrypted filesystem driver for DOS on 386+ PCs, written by Peter Gutmann.
https://aeb.win.tue.nl/partitions/partition_types-1.html
https://www.cs.auckland.ac.nz/~pgut001/sfs/

> +
>              match = re.fullmatch(r'/dev/(?P<ctrler>nvme\d+)n\d', devname)
>              if match is not None:
>                  entry['nvme_controller'] = f'nvme-controller-{match["ctrler"]}'
> @@ -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,

The general class of problem is that this partition type is unsupported, and we may encounter other such partition types.  What do you think about a different field name to indicate that it's unsupported, and check for that below?  I'm attempting to avoid a scenario where we need to `if ptable and not part.get("on-dynamic-disk") and not part.get("other-thing")`

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