Juan Hernandez has posted comments on this change.
Change subject: packaging: change refence to new sysctl.conf location
......................................................................
Patch Set 3: (2 inline comments)
I think this is the right thing to do, just some comments on the way to do it.
....................................................
File packaging/fedora/setup/basedefs.py
Line 105: FILE_VIRTIO_WIN_ISO="/usr/share/virtio-win/virtio-win.iso"
Line 106:
FILE_RHEV_GUEST_TOOLS_ISO="/usr/share/rhev-guest-tools-iso/rhev-tools-setup.iso"
Line 107: FILE_SYSCTL="/etc/sysctl.conf"
Line 108: DIR_SYSCTL="/etc/sysctl.d"
Line 109: FILE_ENGINE_SYSCTL=os.path.join(DIR_SYSCTL,"00-ovirt-engine.conf")
Add an space after the comma.
Also usually the "=" in assignments should be surrounded by spaces.
I would also suggest to add a comment and put all the three constants under it:
# Locations of the kernel configuration files:
FILE_SYSCTL = "..."
DIR_SYSCTL = "..."
FILE_ENGINE_SYSCTL = "..."
Line 110:
Line 111: # ISO
Line 112: ISO_DISPLAY_NAME = "ISO_DOMAIN"
Line 113: DEFAULT_ISO_EXPORT_PATH = "/var/lib/exports/iso"
....................................................
File packaging/fedora/setup/engine-setup.py
Line 1623: if currentShmmax and (int(currentShmmax) >=
basedefs.CONST_SHMMAX):
Line 1624: logging.debug("current shared memory max in kernel is %s,
there is no need to update the kernel parameters", currentShmmax)
Line 1625: return
Line 1626:
Line 1627: # In some systems the sysctl file doesn't exist, so we may need
to create it:
Instead of checking for the existence of the sysctl.conf file two times I would
use a local variable to store the location of the file that we are going to
modify:
# Decide what is the file that we are going to modify according to the
# existence or not of sysctl.conf, it exists in older distributions but
# has been replaced by a sysctl.d directory in newer distributions
# using systemd:
sysctlFile = basedefs.FILE_SYSCTL
if not os.exists(sysctlFile):
sysctlFile = basedefs.FILE_ENGINE_SYSCTL
# Create the chosen file if it doesn't exist:
if not os.exists(sysctlFile):
open(sysctlFile, "w").close()
Etc.
Line 1628: if not os.path.exists(basedefs.FILE_SYSCTL):
Line 1629: open(basedefs.FILE_ENGINE_SYSCTL, "w").close()
Line 1630:
Line 1631: # If we got here, it means we need to update kernel.shmmax in
sysctl.conf
--
To view, visit http://gerrit.ovirt.org/8705
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ca4988beea1627b2da442a7fcf8fef3cf3db6a5
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ohad Basan <[email protected]>
Gerrit-Reviewer: Alex Lourie <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Moran Goldboim <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: Ohad Basan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
