curtin-dev team mailing list archive

[Ryan Harper <rharper@xxxxxxxxxxx>]

Definitely should add a less than 1MB partition in one of the vmtests
Comment below, I suggest that we drop the strict=true in clear_holders.py:_wipe_superblock() instead.

Diff comments:

> diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
> index 1b33002..ae85da9 100644
> --- a/curtin/block/__init__.py
> +++ b/curtin/block/__init__.py
> @@ -1209,14 +1209,24 @@ def quick_zero(path, partitions=True, exclusive=True, strict=False):
>      if this is a block device and partitions is true, then
>      zero 1M at front and end of each partition.
>      """
> -    buflen = 1024
> -    count = 1024
> -    zero_size = buflen * count
> -    offsets = [0, -zero_size]
>      is_block = is_block_device(path)
>      if not (is_block or os.path.isfile(path)):
>          raise ValueError("%s: not an existing file or block device", path)
>  
> +    if is_block:
> +        dev_size = read_sys_block_size_bytes(path_to_kname(path))
> +    else:
> +        dev_size = os.path.getsize(path)

This means we're going to read the target twice, once here and once again in zero_file_at_offset.  There's already some logic there which will truncate the range to wipe and instead of raising the value error just log the warning.  This is controlled by strict= parameter which in clear_holders is set in _wipe_superblock which I think we can drop; clear-holders is doing it's best to wipe all the things so we don't really care how it's done.

Looking at git history, _wipe_superblock added strict=true when we added DASD support, and unfortunately I provided *no* justification for this change:

commit f722ef6b6bffadbe0ac5c3e7b22ae77bf8ab297d
Author: Ryan Harper <ryan.harper@xxxxxxxxxxxxx>
Date:   Mon Apr 1 14:31:19 2019 +0000

    Add support for s390 DASD devices

With that said, I'd support just dropping the strict=True in curtin/block/clear_holders.py

diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
index c182d91a..ba026574 100644
--- a/curtin/block/clear_holders.py
+++ b/curtin/block/clear_holders.py
@@ -332,7 +332,7 @@ def wipe_superblock(device):
             time.sleep(wait)
 
 
-def _wipe_superblock(blockdev, exclusive=True, strict=True):
+def _wipe_superblock(blockdev, exclusive=True):
     """ No checks, just call wipe_volume """
 
     retries = [1, 3, 5, 7]
@@ -341,8 +341,7 @@ def _wipe_superblock(blockdev, exclusive=True, strict=True):
         LOG.debug('wiping %s attempt %s/%s',
                   blockdev, attempt + 1, len(retries))
         try:
-            block.wipe_volume(blockdev, mode='superblock',
-                              exclusive=exclusive, strict=strict)
+            block.wipe_volume(blockdev, mode='superblock', exclusive=exclusive)
             LOG.debug('successfully wiped device %s on attempt %s/%s',
                       blockdev, attempt + 1, len(retries))
             return

> +    buflen = 1024
> +    count = 1024
> +    zero_size = buflen * count
> +
> +    if zero_size < dev_size:
> +        offsets = [0, -zero_size]
> +    else:
> +        zero_size = dev_size
> +        offsets = [0]
> +
>      pt_names = []
>      if partitions and is_block:
>          ptdata = sysfs_partition_data(path)


-- 
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/399893
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:lp-1868177 into curtin:master.