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
vdsm-patches@lists.fedorahosted.org