Yaniv Bronhaim has posted comments on this change. Change subject: gluster: set selinux labels while creating bricks ......................................................................
Patch Set 4: (4 comments) https://gerrit.ovirt.org/#/c/62773/4/lib/vdsm/gluster/exception.py File lib/vdsm/gluster/exception.py: Line 481: Line 482: class GlusterHostFailedToSetSelinuxContext(GlusterHostException): Line 483: code = 4420 Line 484: Line 485: def __init__(self, brickMountPoint=None, rc=0, out=(), err=()): why having default None? the print will be redundant when its None Line 486: self.rc = rc Line 487: self.out = out Line 488: self.err = err Line 489: self.message = "Failed to set selinux context on the brick : %s" \ https://gerrit.ovirt.org/#/c/62773/4/vdsm/gluster/storagedev.py File vdsm/gluster/storagedev.py: Line 57: _semanageCommandPath = utils.CommandPath("semanage", Line 58: "/sbin/semanage", Line 59: "/usr/sbin/semanage",) Line 60: _restoreconCommandPath = utils.CommandPath("restorecon", Line 61: "/sbin/restorecon", in gluster/api.py you use selinux.restorecon . why here you don't use the selinux package? Line 62: "/usr/sbin/restorecon",) Line 63: Line 64: # All size are in MiB unless otherwise specified Line 65: DEFAULT_CHUNK_SIZE_KB = 256 Line 58: "/sbin/semanage", Line 59: "/usr/sbin/semanage",) Line 60: _restoreconCommandPath = utils.CommandPath("restorecon", Line 61: "/sbin/restorecon", Line 62: "/usr/sbin/restorecon",) we also have RESTORECON_PATH - maybe you can fix RESTORECON_PATH's paths and use it? Line 63: Line 64: # All size are in MiB unless otherwise specified Line 65: DEFAULT_CHUNK_SIZE_KB = 256 Line 66: DEFAULT_METADATA_SIZE_KB = 16777216 Line 331: Line 332: rc, out, err = commands.execCmd([_restoreconCommandPath.cmd, Line 333: '-Rv', mountPoint]) Line 334: if rc: Line 335: raise ge.GlusterHostFailedToRunRestorecon(mountPoint, rc, out, err) maybe this should be part of vdsm-tool configure sebool (configurators/sebool.py) ? if selinux was disabled when this function was called but later turned to enabled - nothing will set the context. in vdsm documentation we should ask users to run vdsm-tool configure after changing selinux state Line 336: -- To view, visit https://gerrit.ovirt.org/62773 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ca5fec80831073643635875095b88c1c4c2132e Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
