Francesco Romani has posted comments on this change.
Change subject: lib: grub2 module ......................................................................
Patch Set 3:
(9 comments)
few comments, -1 for visibility
Please add one-liners (or more, of course, if you need) defining "workaround" and "stub".
https://gerrit.ovirt.org/#/c/47946/3/lib/vdsm/grub2.py File lib/vdsm/grub2.py:
Line 45: return "Process failed with rc=%d out=%r err=%r" % ( Line 46: self.rc, self.out, self.err) Line 47: Line 48: Line 49: def mkconfig(args): please add docstrings to functions. No need to document every parameter or to be verbose, just outline what functions should do.
Do we have a wiki page or anything describing the flow? I mean the intended flow, not the implemented flow. Line 50: cmd = [_GRUB2_MKCONFIG.cmd] Line 51: cmd.extend(args) Line 52: rc, out, err = utils.execCmd(_GRUB2_MKCONFIG, raw=True) Line 53: if rc != 0:
Line 160: Line 161: device_vendor = device_vendor.lstrip('0x') Line 162: device_product = device_product.lstrip('0x') Line 163: Line 164: if stub_ids: does the ordering matter? can we use a set? Line 165: for vendor, product in stub_ids: Line 166: if device_vendor == vendor and device_product == product: Line 167: return Line 168:
Line 163: Line 164: if stub_ids: Line 165: for vendor, product in stub_ids: Line 166: if device_vendor == vendor and device_product == product: Line 167: return this check you implemented is duplicating the default behaviour of python. I think you can just check like this:
if [vendor, product] in stub_ids: return Line 168: Line 169: stub_ids.append([device_vendor, device_product]) Line 170: Line 171:
Line 165: for vendor, product in stub_ids: Line 166: if device_vendor == vendor and device_product == product: Line 167: return Line 168: Line 169: stub_ids.append([device_vendor, device_product]) why list and not tuple? Line 170: Line 171: Line 172: def _del_from_stub(stub_ids, device_vendor, device_product): Line 173:
Line 173: Line 174: device_vendor = device_vendor.lstrip('0x') Line 175: device_product = device_product.lstrip('0x') Line 176: Line 177: if stub_ids: does it work without the if? To iterate in an empty collection should exit immediately. Line 178: for idx, (vendor, product) in enumerate(stub_ids): Line 179: if device_vendor == vendor and device_product == product: Line 180: del(stub_ids[idx]) Line 181: break
Line 193: Line 194: Line 195: def _add_workaround(workarounds, workaround): Line 196: try: Line 197: workarounds.index(workaround) same: do we care about the ordering? If so, please document it why. Otherwise, let's just use a set Line 198: except ValueError: Line 199: workarounds.append(workaround) Line 200: Line 201:
https://gerrit.ovirt.org/#/c/47946/3/tests/grub2Tests.py File tests/grub2Tests.py:
Line 55: Line 56: self._assertIntegrity(data) Line 57: self.assertIn('"pci-stub.ids=cafe:beef"', data) Line 58: Line 59: def testAddDuplicateStub2(self): please use a more describing name, and higlight the differences with respect to the other test Line 60: with tempfile.NamedTemporaryFile() as f: Line 61: grub2.add_to_stub( Line 62: ['cafe', '0xcafe'], ['beef', '0xbeef'], Line 63: cfg_source=self._test_file('grub2_default.out'),
Line 71: def testAddMultipleStubs(self): Line 72: with tempfile.NamedTemporaryFile() as f: Line 73: grub2.add_to_stub( Line 74: ['cafe', '0xdead', '0xf00d', 'b002'], Line 75: ['0xbeef', 'feed', '0xc00l', 'fa11'], I missed this reviewing the code! so question: why is it helpful to support lists and not just one argument? I mean, what's the use case?
Furthermore, maybe having a single list of pairs (tuples) is a bit more easier to read. Did you try that approach? Line 76: cfg_source=self._test_file('grub2_default.out'), Line 77: cfg_dest=f.name) Line 78: Line 79: data = f.read()
Line 169: Line 170: self._assertIntegrity(data) Line 171: self.assertIn('VDSM_WORKAROUNDS="a=b"', data) Line 172: Line 173: def testAddDuplicateWorkaround2(self): same as for stub Line 174: with tempfile.NamedTemporaryFile() as f: Line 175: grub2.add_workaround( Line 176: ['a=b', 'a=b'], Line 177: cfg_source=self._test_file('grub2_default.out'),