Francesco Romani has posted comments on this change.
Change subject: lib: grub2 module ......................................................................
Patch Set 1:
(8 comments)
initial review, mostly minor things. To be able to do a meaningful review, I miss to see how this is going to be used. I'd like to ask for
- tests! - a simple commandline tool to exercise/run the same code without vdsm running,for debug/troubleshooting purpose. Let's have it under contrib/
https://gerrit.ovirt.org/#/c/47946/1//COMMIT_MSG Commit Message:
Line 5: CommitDate: 2015-11-02 10:49:59 +0100 Line 6: Line 7: lib: grub2 module Line 8: Line 9: work in progress when you add the real content, please say it loud and clear that in the long term we want to depend on a system configuration tool, asking just for the things we care, and state this is a short-term quick fix until we can use such tool. Line 10: Line 11: Change-Id: I9e7e0b327882f64196a71fe20dcbd99017b80800
https://gerrit.ovirt.org/#/c/47946/1/lib/vdsm/grub2.py File lib/vdsm/grub2.py:
Line 29: _CFG_NAME_WORKAROUNDS = 'VDSM_WORKAROUNDS' Line 30: _CFG_NAME_CMDLINE = 'GRUB_CMDLINE_LINUX' Line 31: _CMDLINE_STUB_IDS = 'pci-stub.ids=' Line 32: Line 33: _GRUB2_MKCONFIG = utils.CommandPath('/usr/sbin/grub2-mkconfig')
maybe just grub2-mkconfig and let CommandPath figure out the path?
Nope, the common idiom is exactly this one. Bogus comment from me, let's ignore it. Line 34: Line 35: Line 36: class Error(Exception): Line 37:
Line 32: Line 33: _GRUB2_MKCONFIG = utils.CommandPath('/usr/sbin/grub2-mkconfig') Line 34: Line 35: Line 36: class Error(Exception):
This is exactly what I'd like to see as I've seen it 3-or-so times lately.
Tracked in https://gerrit.ovirt.org/#/c/47964/ - and following Line 37: Line 38: def __init__(self, rc, out, err): Line 39: self.rc = rc Line 40: self.out = out
Line 51: rc, out, err = utils.execCmd(_GRUB2_MKCONFIG, raw=True) Line 52: if rc != 0: Line 53: raise Error(rc, out, err) Line 54: Line 55: no other entry points? How the next functions are going to be used? Line 56: def _init_cfg(grub_cfg): Line 57: if _CFG_NAME_PCI_STUB not in grub_cfg: Line 58: new_cfg = collections.OrderedDict() Line 59: new_cfg[_CFG_NAME_PCI_STUB] = _CMDLINE_STUB_IDS
Line 81: for line in data: Line 82: line = line.rstrip().split('=', 1) Line 83: grub_cfg[line[0]] = line[1] Line 84: Line 85: return grub_cfg minor: why not just
def _parse_cfg(cfg=_GRUB_CONFIG): grub_cfg = collections.OrderedDict()
with open(cfg, 'r') as f: for line in f: key, value = line.rstrip().split('=', 1) grub_cfg[key] = value
return grub_cfg Line 86: Line 87: Line 88: def _save_cfg(grub_cfg, cfg=_GRUB_CONFIG): Line 89: with open(cfg, 'w') as f:
Line 115: Line 116: device_vendor = device_vendor.lstrip('0x') Line 117: device_product = device_product.lstrip('0x') Line 118: Line 119: if len(stub_ids) > 0: if stub_ids: Line 120: for vendor, product in stub_ids: Line 121: if device_vendor == vendor and device_product == product: Line 122: return Line 123:
Line 128: Line 129: device_vendor = device_vendor.lstrip('0x') Line 130: device_product = device_product.lstrip('0x') Line 131: Line 132: if len(stub_ids) > 0: ditto Line 133: for idx, (vendor, product) in enumerate(stub_ids): Line 134: if device_vendor == vendor and device_product == product: Line 135: del(stub_ids[idx]) Line 136: break
Line 143: if not val: Line 144: continue Line 145: workarounds.append(val) Line 146: Line 147: return workarounds could be a list comprehnsion Line 148: Line 149: Line 150: def _add_workaround(workarounds, workaround): Line 151: try:
vdsm-patches@lists.fedorahosted.org