Change in vdsm[master]: lib: grub2 module

[automation]

gerrit-hooks has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found

-- 
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[mpolednik]

Martin Polednik has abandoned this change.

Change subject: lib: grub2 module
..


Abandoned

-- 
To view, visit https://gerrit.ovirt.org/47946
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I9e7e0b327882f64196a71fe20dcbd99017b80800
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[automation]

gerrit-hooks has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

-- 
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 3:

(3 comments)

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 
Added. No wiki yet, let me think about it.
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?
Possibly!
Line 165: for vendor, product in stub_ids:
Line 166: if device_vendor == vendor and device_product == product:
Line 167: return
Line 168: 


https://gerrit.ovirt.org/#/c/47946/3/tests/grub2Tests.py
File tests/grub2Tests.py:

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
Discussed with engine, that will be the approach for next revision.
Line 76: cfg_source=self._test_file('grub2_default.out'),
Line 77: cfg_dest=f.name)
Line 78: 
Line 79: data = f.read()


-- 
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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 5:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 4:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[fromani]

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: 

Change in vdsm[master]: lib: grub2 module

[fromani]

Francesco Romani has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/47946/1/lib/vdsm/grub2.py
File lib/vdsm/grub2.py:

Line 143: if not val:
Line 144: continue
Line 145: workarounds.append(val)
Line 146: 
Line 147: return workarounds
> Not really, as split would introduce empty element.
uhm I probably miss something but:

  workarounds = [
val for val in (grub_cfg[_CFG_NAME_WORKAROUNDS].strip('"').split(' '))
if val
  ]

?

btw, it is very minor
Line 148: 
Line 149: 
Line 150: def _add_workaround(workarounds, workaround):
Line 151: try:


-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Martin Betak 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[fromani]

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:


-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 1:

(2 comments)

Why do you see grub2-* as too low level?

https://gerrit.ovirt.org/#/c/47946/1/lib/vdsm/grub2.py
File lib/vdsm/grub2.py:

Line 22: import collections
Line 23: 
Line 24: from . import utils
Line 25: 
Line 26: _GRUB_CONFIG = os.path.abspath('/etc/default/grub')
> but this isn't already an abs path? :)
I'd say you're right, not really needed to normalize it I guess.
Line 27: 
Line 28: _CFG_NAME_PCI_STUB = 'VDSM_PCI_STUB_DEVICES'
Line 29: _CFG_NAME_WORKAROUNDS = 'VDSM_WORKAROUNDS'
Line 30: _CFG_NAME_CMDLINE = 'GRUB_CMDLINE_LINUX'


Line 32: 
Line 33: _GRUB2_MKCONFIG = utils.CommandPath('/usr/sbin/grub2-mkconfig')
Line 34: 
Line 35: 
Line 36: class Error(Exception):
> For another patch, not necessarily from yours: let's factor this Error copi
This is exactly what I'd like to see as I've seen it 3-or-so times lately.
Line 37: 
Line 38: def __init__(self, rc, out, err):
Line 39: self.rc = rc
Line 40: self.out = 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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
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 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[mpolednik]

Martin Polednik has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/47946/1/lib/vdsm/grub2.py
File lib/vdsm/grub2.py:

Line 143: if not val:
Line 144: continue
Line 145: workarounds.append(val)
Line 146: 
Line 147: return workarounds
> could be a list comprehnsion
Not really, as split would introduce empty element.
Line 148: 
Line 149: 
Line 150: def _add_workaround(workarounds, workaround):
Line 151: try:


-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 
'ovirt-3.4', 'ovirt-3.3'])

-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: lib: grub2 module

[fromani]

Francesco Romani has posted comments on this change.

Change subject: lib: grub2 module
..


Patch Set 1: Code-Review-1

(3 comments)

-1 for visibility

I started reviewing, and gave initial comments, than realized that here we are 
partially replicating a facility that should be system wide.

So, +1 for the feature, but I feel that we need to draw a line, construct the 
parameters we want (or need), but then the actual job of handling the grub2 
config should _enterely_ shed on some other system wide tool (grub2-mkconfig is 
not enough, too low level), on which we should depend on.

https://gerrit.ovirt.org/#/c/47946/1/lib/vdsm/grub2.py
File lib/vdsm/grub2.py:

Line 22: import collections
Line 23: 
Line 24: from . import utils
Line 25: 
Line 26: _GRUB_CONFIG = os.path.abspath('/etc/default/grub')
but this isn't already an abs path? :)
Line 27: 
Line 28: _CFG_NAME_PCI_STUB = 'VDSM_PCI_STUB_DEVICES'
Line 29: _CFG_NAME_WORKAROUNDS = 'VDSM_WORKAROUNDS'
Line 30: _CFG_NAME_CMDLINE = 'GRUB_CMDLINE_LINUX'


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?
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):
For another patch, not necessarily from yours: let's factor this Error copied 
everywhere once for all.
Line 37: 
Line 38: def __init__(self, rc, out, err):
Line 39: self.rc = rc
Line 40: self.out = 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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches