I've left a couple of comments inline, but I'll have to revisit this on Monday
to complete my review.
Diff comments:
> diff --git a/curtin/block/__init__.py b/curtin/block/__init__.py
> index a7fe22f..97596e1 100644
> --- a/curtin/block/__init__.py
> +++ b/curtin/block/__init__.py
> @@ -248,46 +248,47 @@ def _lsblock(args=None):
> return _lsblock_pairs_to_dict(out)
>
>
> -def _sfdisk_parse(lines):
> - info = {}
> - for line in lines:
> - if ':' not in line:
> - continue
> - lhs, _, rhs = line.partition(':')
> - key = lhs.strip()
> - value = rhs.strip()
> - if "," in rhs:
> - value = dict((item.split('=')
> - for item in rhs.replace(' ', '').split(',')))
> - info[key] = value
> -
> - return info
> -
> -
> def sfdisk_info(devpath):
> ''' returns dict of sfdisk info about disk partitions
> {
> - "/dev/vda1": {
> - "size": "20744159",
> - "start": "227328",
> - "type": "0FC63DAF-8483-4772-8E79-3D69D8477DE4",
> - "uuid": "29983666-2A66-4F14-8533-7CE13B715462"
> - },
> - "device": "/dev/vda",
> - "first-lba": "34",
> - "label": "gpt",
> - "label-id": "E94FCCFE-953D-4D4B-9511-451BBCC17A9A",
> - "last-lba": "20971486",
> - "unit": "sectors"
> + "label": "gpt",
> + "id": "877716F7-31D0-4D56-A1ED-4D566EFE418E",
> + "device": "/dev/vda",
> + "unit": "sectors",
> + "firstlba": 34,
> + "lastlba": 41943006,
> + "partitions": [
> + {"node": "/dev/vda1", "start": 227328, "size": 41715679,
> + "type": "0FC63DAF-8483-4772-8E79-3D69D8477DE4",
> + "uuid": "60541CAF-E2AC-48CD-BF89-AF16051C833F"},
> + ]
> + }
> + {
> + "label":"dos",
> + "id":"0xb0dbdde1",
> + "device":"/dev/vdb",
> + "unit":"sectors",
> + "partitions": [
> + {"node":"/dev/vdb1", "start":2048, "size":8388608,
> + "type":"83", "bootable":true},
> + {"node":"/dev/vdb2", "start":8390656, "size":8388608, "type":"83"},
> + {"node":"/dev/vdb3", "start":16779264, "size":62914560, "type":"5"},
> + {"node":"/dev/vdb5", "start":16781312, "size":31457280,
> "type":"83"},
> + {"node":"/dev/vdb6", "start":48240640, "size":10485760,
> "type":"83"},
> + {"node":"/dev/vdb7", "start":58728448, "size":20965376, "type":"83"}
> + ]
> }
> '''
> (parent, partnum) = get_blockdev_for_partition(devpath)
> try:
> - (out, _err) = util.subp(['sfdisk', '--dump', parent], capture=True)
> + (out, _err) = util.subp(['sfdisk', '--json', parent], capture=True)
`--json` was introduced in util-linux 2.27[0] which means it isn't present
pre-xenial[1] or in Centos 7[2]. Is that an issue?
[0]
https://github.com/karelzak/util-linux/blob/master/Documentation/releases/v2.27-ReleaseNotes#L611
[1] https://launchpad.net/ubuntu/+source/util-linux
[2] Assuming that the version listed on
http://mirror.centos.org/centos/7/os/x86_64/Packages/ is the version in Centos
7, that is.
> except util.ProcessExecutionError as e:
> + out = None
> LOG.exception(e)
> - out = ""
> - return _sfdisk_parse(out.splitlines())
> + if out is not None:
> + return util.load_json(out).get('partitiontable', {})
> +
> + return {}
>
>
> def dmsetup_info(devname):
> diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
> index 1ad49b7..52eae94 100644
> --- a/curtin/commands/block_meta.py
> +++ b/curtin/commands/block_meta.py
> @@ -725,19 +751,30 @@ def verify_size(devpath, expected_size_bytes):
> raise RuntimeError(msg)
>
>
> -def verify_ptable_flag(devpath, expected_flag):
> - if not SGDISK_FLAGS.get(expected_flag):
> +def verify_ptable_flag(devpath, expected_flag, sfdisk_info=None):
> + if expected_flag not in (list(SGDISK_FLAGS.keys()) +
This would be neater as:
if expected_flag not in SGDISK_FLAGS and expected_flag not in MSDOS_FLAGS:
Python won't even evaluate `expected_flag not in MSDOS_FLAGS` if expected_flag
is in SGDISK_FLAGS, but even if it does it's considerably more performant on
empty dicts:
$ python3 -m timeit -s "a, b = {}, {}" "'a' in list(a.keys()) +
list(b.keys())"
1000000 loops, best of 5: 317 nsec per loop
$ python3 -m timeit -s "a, b = {}, {}" "'a' not in a and 'a' not in b"
5000000 loops, best of 5: 45.1 nsec per loop
and that difference will get more stark as the dicts increase in size, because
checking for dict membership is pretty much O(1) whereas the current code is
~O(n). This is what they look like run across two dicts with 1000 entries ({1:
1, 2: 2, etc}):
$ python3 -m timeit -s "a, b = {k: v for k, v in zip(range(1000),
range(1000))}, {k: v for k, v in zip(range(1000), range(1000))}" "'a' in
list(a.keys()) + list(b.keys())"
10000 loops, best of 5: 30.9 usec per loop
$ python3 -m timeit -s "a, b = {k: v for k, v in zip(range(1000),
range(1000))}, {k: v for k, v in zip(range(1000), range(1000))}" "'a' not in a
and 'a' not in b"
5000000 loops, best of 5: 46.4 nsec per loop
(Got a little carried away here, as you can probably tell. ;)
> + list(MSDOS_FLAGS.keys())):
> raise RuntimeError(
> - 'Cannot verify unknown partition flag: %s', expected_flag)
> + 'Cannot verify unknown partition flag: %s' % expected_flag)
>
> - info = block.sfdisk_info(devpath)
> - if devpath not in info:
> - raise RuntimeError('Device %s not present in sfdisk dump:\n%s' %
> - devpath, util.json_dumps(info))
> + if not sfdisk_info:
> + sfdisk_info = block.sfdisk_info(devpath)
> + if not sfdisk_info:
> + raise RuntimeError('Failed to extract sfdisk info from %s' % devpath)
>
> - entry = info[devpath]
> + entry = _sfdisk_get_partition_info(devpath, sfdisk_info=sfdisk_info)
> LOG.debug("Device %s ptable entry: %s", devpath, util.json_dumps(entry))
> - (found_flag, code) = ptable_uuid_to_flag_entry(entry['type'])
> + found_flag = None
> + if (sfdisk_info['label'] in ('dos', 'msdos')):
> + if expected_flag == 'boot':
> + found_flag = 'boot' if entry.get('bootable') is True else None
> + elif expected_flag == 'extended':
> + (found_flag, _code) = ptable_uuid_to_flag_entry(entry['type'])
> + elif expected_flag == 'logical':
> + (_parent, partnumber) = block.get_blockdev_for_partition(devpath)
> + found_flag = 'logical' if int(partnumber) > 4 else None
> + else:
> + (found_flag, _code) = ptable_uuid_to_flag_entry(entry['type'])
> msg = (
> 'Verifying %s partition flag, expecting %s, found %s' % (
> devpath, expected_flag, found_flag))
--
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/383180
Your team curtin developers is subscribed to branch curtin:master.
--
Mailing list: https://launchpad.net/~curtin-dev
Post to : curtin-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~curtin-dev
More help : https://help.launchpad.net/ListHelp
