curtin-dev team mailing list archive

[Michael Hudson-Doyle <mp+443380@xxxxxxxxxxxxxxxxxx>]

Review: Approve

a lot of groaning but ok.

Diff comments:

> diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
> index 2f2b60c..0e13a0d 100644
> --- a/curtin/block/__init__.py
> +++ b/curtin/block/__init__.py
> @@ -1041,9 +1041,21 @@ def check_dos_signature(device):
>      # this signature must be at 0x1fe
>      # https://en.wikipedia.org/wiki/Master_boot_record#Sector_layout
>      devname = dev_path(path_to_kname(device))
> -    return (is_block_device(devname) and util.file_size(devname) >= 0x200 and
> -            (util.load_file(devname, decode=False, read_len=2, offset=0x1fe) ==
> -             b'\x55\xAA'))
> +    if not is_block_device(devname):
> +        return False
> +    try:
> +        # Some older series have the extended partition block device but return
> +        # ENXIO when attempting to read it.
> +        file_size = util.file_size(devname)
> +    except OSError as ose:
> +        if ose.errno == errno.ENXIO:

Er the way you phrase the comment, it sounds like you should return True here? I can believe that's not the right thing to do but perhaps a little more ranting in comments is appropriate?

> +            return False
> +        else:
> +            raise
> +    if file_size < 0x200:
> +        return False
> +    signature = util.load_file(devname, decode=False, read_len=2, offset=0x1fe)
> +    return signature == b'\x55\xAA'
>  
>  
>  def check_efi_signature(device):
> diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
> index 7988f3a..71035f9 100644
> --- a/curtin/commands/block_meta.py
> +++ b/curtin/commands/block_meta.py
> @@ -854,17 +854,28 @@ def calc_dm_partition_info(partition_kname):
>  
>  
>  def calc_partition_info(partition_kname, logical_block_size_bytes):
> +    p_size_sec = 0
> +    p_start_sec = 0
>      if partition_kname.startswith('dm-'):
>          p_start, p_size = calc_dm_partition_info(partition_kname)
>      else:
>          pdir = block.sys_block_path(partition_kname)
>          p_size = int(util.load_file(os.path.join(pdir, "size")))
>          p_start = int(util.load_file(os.path.join(pdir, "start")))
> +        if not all([p_size, p_start]):

can you just write this as "p_size == 0 and p_start == 0" please?

> +            # if sysfs reported a 0, let's try sfdisk
> +            sfdisk_info = block.sfdisk_info(partition_kname)
> +            part_path = block.kname_to_path(partition_kname)
> +            part_info = block.get_partition_sfdisk_info(part_path, sfdisk_info)
> +            p_size_sec = part_info['size']
> +            p_start_sec = part_info['start']
>  
>      # NB: sys/block/X/{size,start} and dmsetup output are both always
>      # in 512b sectors
> -    p_size_sec = p_size * 512 // logical_block_size_bytes
> -    p_start_sec = p_start * 512 // logical_block_size_bytes
> +    if p_size_sec == 0:
> +        p_size_sec = p_size * 512 // logical_block_size_bytes
> +    if p_start_sec == 0:
> +        p_start_sec = p_start * 512 // logical_block_size_bytes
>  
>      LOG.debug("calc_partition_info: %s size_sectors=%s start_sectors=%s",
>                partition_kname, p_size_sec, p_start_sec)


-- 
https://code.launchpad.net/~dbungert/curtin/+git/curtin/+merge/443380
Your team curtin developers is subscribed to branch curtin:master.