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'),
--
To view, visit
https://gerrit.ovirt.org/47946
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e7e0b327882f64196a71fe20dcbd99017b80800
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Martin Betak <mbetak(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes