curtin-dev team mailing list archive

[Dimitri John Ledkov <dimitri.ledkov+lp@xxxxxxxxxxxxx>]

Review: Approve

This is fine refactor.

Diff comments:

> diff --git a/curtin/block/dasd.py b/curtin/block/dasd.py
> index b7008f6..e74e510 100644
> --- a/curtin/block/dasd.py
> +++ b/curtin/block/dasd.py
> @@ -435,31 +437,24 @@ class DasdDevice(CcwDevice):
>          if strict and not os.path.exists(self.devname):
>              raise RuntimeError("devname '%s' does not exist" % self.devname)
>  
> -        info = dasdview(self.devname)
> -        geo = info['geometry']
> -
> -        existing_partitions = self.get_partition_table()
> -        partitions = []
> -        for partinfo in existing_partitions[0:partnumber]:
> -            # (devpath, start_track, end_track, nr_tracks, partnum)
> -            start = partinfo[1]
> -            end = partinfo[2]
> -            partitions.append((start, end))
> +        pt = DasdPartitionTable.from_fdasd(self.devname)

my monkey brain goes "it doesn't have partition table, it has vtoc, variable name should be vtoc too not pt...." and yet look at fdasd arguments, it uses "--table" *sigh*.

> +        new_partitions = []
> +        for partinfo in pt.partitions[0:partnumber]:
> +            new_partitions.append((partinfo.start, partinfo.end))
>  
>          # first partition always starts at track 2
>          # all others start after the previous partition ends
>          if partnumber == 1:
>              start = 2
>          else:
> -            start = int(partitions[-1][1]) + 1
> -        # end is size + 1
> -        tracks_needed = int(self._bytes_to_tracks(geo, partsize))
> -        end = start + tracks_needed + 1
> -        partitions.append(("%s" % start, "%s" % end))
> +            start = int(pt.partitions[-1].end) + 1
> +        # end is inclusive
> +        end = start + pt.tracks_needed(partsize) - 1
> +        new_partitions.append((start, end))

yes, that's better way to calculate the end.

>  
>          content = "\n".join(["[%s,%s]" % (part[0], part[1])
> -                             for part in partitions])
> -        LOG.debug("fdasd: partitions to be created: %s", partitions)
> +                             for part in new_partitions])
> +        LOG.debug("fdasd: partitions to be created: %s", new_partitions)
>          LOG.debug("fdasd: content=\n%s", content)
>          wfp = tempfile.NamedTemporaryFile(suffix=".fdasd", delete=False)
>          wfp.close()


-- 
https://code.launchpad.net/~mwhudson/curtin/+git/curtin/+merge/393791
Your team curtin developers is requested to review the proposed merge of ~mwhudson/curtin:partition-verify-dasd into curtin:master.