curtin-dev team mailing list archive

[Olivier Gayot <mp+469319@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

Thanks! It makes sense to me to wipe the partitions before the disk.

Some minor comments that we should ideally address and then LGTM

Diff comments:

> diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
> index 2e19467..39cf1cc 100644
> --- a/curtin/block/__init__.py
> +++ b/curtin/block/__init__.py
> @@ -1000,21 +1000,28 @@ def sysfs_partition_data(blockdev=None, sysfs_path=None):
>          sysfs_prefix = sys_block_path(parent)
>          partnum = int(partnum)
>  
> +    keys = {'partition', 'start', 'size'}
>      ptdata = []
>      for part_sysfs in get_sysfs_partitions(sysfs_prefix):
>          data = {}
> -        for sfile in ('partition', 'start', 'size'):
> +        for sfile in keys:
>              dfile = os.path.join(part_sysfs, sfile)
>              if not os.path.isfile(dfile):
>                  continue
>              data[sfile] = int(util.load_file(dfile))
>          if partnum is None or data['partition'] == partnum:
> -            ptdata.append((
> -                path_to_kname(part_sysfs),
> -                data['partition'],
> -                data['start'] * SECTOR_SIZE_BYTES,
> -                data['size'] * SECTOR_SIZE_BYTES,
> -                ))
> +            if data.keys() == keys:
> +                ptdata.append((
> +                    path_to_kname(part_sysfs),
> +                    data['partition'],
> +                    data['start'] * SECTOR_SIZE_BYTES,
> +                    data['size'] * SECTOR_SIZE_BYTES,
> +                    ))
> +            else:
> +                LOG.debug(
> +                    "sysfs_partition_data: "
> +                    "skipping {part_sysfs} - incomplete sysfs read"

Missing f-string?

> +                )
>  
>      return ptdata
>  
> @@ -1231,7 +1238,6 @@ def quick_zero(path, partitions=True, exclusive=True):
>      if this is a block device and partitions is true, then

Could you update this docstring to reflect the change(s)?

>      zero 1M at front and end of each partition.
>      """
> -    util.subp(['wipefs', '--all', '--force', path])
>      buflen = 1024
>      count = 1024
>      zero_size = buflen * count
> @@ -1257,6 +1263,8 @@ def quick_zero(path, partitions=True, exclusive=True):
>              zero_file_at_offsets,
>              path, offsets, buflen=buflen, count=count, exclusive=exclusive)
>  
> +    util.subp(['wipefs', '--all', '--force', path])

I'm wondering if this should be moved before `zero_file_at_offsets`? Wdyt?

> +
>  
>  def zero_file_at_offsets(path, offsets, buflen=1024, count=1024, strict=False,
>                           exclusive=True):


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