[node-patches] Change in ovirt-node[master]: Add utils.system.Bootloader

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: Add utils.system.Bootloader
......................................................................


Patch Set 3: Code-Review-1

(5 comments)

....................................................
File src/ovirt/node/utils/system.py
Line 92:     """Determines whether a filesystem with a given label is present on this
Line 93:     system
Line 94:     """
Line 95:     try:
Line 96:         process.call(["partprobe", "/dev/mapper/*"])
The expansion (/dev/mapper/*) is typically done by the bash.herefor you need to 
a) use glob to do the expansion yourself
or
b) use a string as the first argument (containing the cmd to be run) and the keyword shell=True - then the expansion will be handled by the invoked shell.
Line 97:         process.call(["udevadm", "settle"])
Line 98:         if process.check_call(["/sbin/blkid", "-c", "/dev/null", "-l",
Line 99:                                "-o", "device", "-t", 'LABEL="%s"' % label
Line 100:                                ]) == 0:


Line 100:                                ]) == 0:
Line 101:             return True
Line 102:         else:
Line 103:             return False
Line 104:     except process.CalledProcessError:
please add a log.exception() call here to not silently swallow an exception
Line 105:         return False
Line 106: 
Line 107: 
Line 108: def which(cmd):


Line 559:                 cfg_path = "/dev/.initramfs/live/grub/grub.conf"
Line 560:             else:
Line 561:                 cfg_path = "/dev/.initramfs/live/grub2/grub.cfg"
Line 562:         elif os.path.ismount("/run/initramfs/.live"):
Line 563:             cfg_path = "/liveos/grub/grub.conf"
please add an else: branch which raises e.g. a RuntimeError - this will help debugging if e.g /dev/.initramfs vanishes at some point ..
Line 564: 
Line 565:         return cfg_path
Line 566: 
Line 567:     def _get_grublines(self):


Line 564: 
Line 565:         return cfg_path
Line 566: 
Line 567:     def _get_grublines(self):
Line 568:         return open(self.grub_cfg, "r").readlines()
Please use utils.fs.File() for the file access.That helps adding unit tests
Line 569: 
Line 570:     def get_args(self, arg=None):
Line 571:         lines = self._get_grublines()
Line 572:         kernel = [line for line in lines if


Line 626:             else:
Line 627:                 utils.process.check_call(["mount", "-o", "ro,remount"
Line 628:                                           "%s" % self.__find_mount(path)])
Line 629:         except:
Line 630:             LOGGER.exception("Can't remount %s!" % path)
please use self.logger - I hope that the global LOGGER will go away at some point ..
Line 631: 
Line 632:     def __find_mount(self, path):
Line 633:         while not os.path.ismount(path) and path != "/":
Line 634:             path = os.path.dirname(path)


-- 
To view, visit http://gerrit.ovirt.org/19811
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a796a4c07bd99b927070ec62a7b1d990d4e1f9b
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes