mooli tayer has posted comments on this change.
Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:
Line 86: P_SYSTEMCTL_CONF = SYSCONF_PATH + '/sysctl.d/vdsm'
Line 87: P_VDSM_LCONF = SYSCONF_PATH + '/libvirt/libvirtd.conf'
Line 88: P_VDSM_LDCONF = SYSCONF_PATH + '/sysconfig/libvirtd'
Line 89: P_VDSM_QCONF = SYSCONF_PATH + '/libvirt/qemu.conf'
Line 90:
no need for that
I guess this is related to your comment on
configurator.py in line 151? my response to you there is that i'm not sure what you
want done in this patch instead?
Line 91: #
Line 92: # External programs (sorted, please keep in order).
Line 93: #
Line 94: EXT_BLKID = '@BLKID_PATH@'
http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 29: from ..constants import P_VDSM_EXEC, DISKIMAGE_GROUP, QEMU_PROCESS_GROUP, \
Line 30: VDSM_GROUP, P_VDSM_LCONF, P_VDSM_QCONF, P_VDSM_LDCONF, P_SYSTEMCTL_CONF
Line 31:
Line 32:
Line 33: def removeSectionFromFile(filename, beginField=None, endField=None):
I do not think it is that complex, and it can be improved (testing
the exac
> testing the exact lines we add instead of signature before/after
I guess that a patch removing exact lines should also change the code that inserts them?
not sure if it should be done now or or in a different patch.
Also, I do not like the duplicated:
'## beginning of configuration section by vdsm' in this patch.
Just a thought: is there a way we can do a one line import inside libvirt configuration
file/s to a new file containing our conf and then just remove that?
Line 34: (
Line 35: BEFORE,
Line 36: WITHIN,
Line 37: AFTER,
Line 147: conf_prefix = '## beginning of configuration section by vdsm' +
\
Line 148: vdsm_ver
Line 149: conf_suffix = '## end of configuration section by vdsm' +
vdsm_ver
Line 150:
Line 151: conf_paths = [
libvirt_configure.sh already keeps those files names in it,
remove_conf ver
I am not sure what you are suggesting to do in this patch instead?
Line 152: P_VDSM_LCONF,
Line 153: P_VDSM_QCONF,
Line 154: P_VDSM_LDCONF,
Line 155: ]
Line 357: m = [
Line 358: c.getName() for c in __configurers
Line 359: if c.getName() in args.modules and not c.removeConf()
Line 360: ]
Line 361: if m:
c.removeConf()
Well I guess this whole section should change
since c.removeConf() can never return false? Some other way should be used to detect
errors.
Line 362: sys.stdout.write(
Line 363: "Could not remove configuration for modules %s\n" %
','.join(m),
Line 364: )
Line 365: ret = False
--
To view, visit
http://gerrit.ovirt.org/21772
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7f2c031436a6d202f856c24d9c9420c8bfdf6df
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes