Change in vdsm[master]: Adding remove\disable verbs to vdsm-tool for admin usages

Alon Bar-Lev alonbl at redhat.com
Tue Feb 25 17:12:37 UTC 2014


Alon Bar-Lev has posted comments on this change.

Change subject: Adding remove\disable verbs to vdsm-tool for admin usages
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.ovirt.org/#/c/21772/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 53:             newcontent.append(line)
Line 54:         else:
Line 55:             raise RuntimeError("Internal Error")
Line 56:     if content != newcontent:
Line 57:         tmpfile = tempfile.NamedTemporaryFile(delete=False)
you need to create temp file at same directory of original file... see tempfile.mkstemp, and then use os.rename for atomic.
Line 58:         tname = tmpfile.name
Line 59:         tmpfile.writelines(newcontent)
Line 60:         tmpfile.close()
Line 61:         shutil.move(tname, filename)


Line 56:     if content != newcontent:
Line 57:         tmpfile = tempfile.NamedTemporaryFile(delete=False)
Line 58:         tname = tmpfile.name
Line 59:         tmpfile.writelines(newcontent)
Line 60:         tmpfile.close()
you should never use close() but python with structure.
Line 61:         shutil.move(tname, filename)
Line 62: 
Line 63: 
Line 64: class _ModuleConfigure(object):


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 = [
consider remove temporary one time used variable.

 for path in  [
 ]:
Line 152:             P_VDSM_LCONF,
Line 153:             P_VDSM_QCONF,
Line 154:             P_VDSM_LDCONF,
Line 155:         ]


Line 156:         for path in conf_paths:
Line 157:             if os.path.exists(path):
Line 158:                 try:
Line 159:                     removeSectionFromFile(path, conf_prefix, conf_suffix)
Line 160:                 except OSError, e:
no need try/except now
Line 161:                     if e.errno != os.errno.EEXIST:
Line 162:                         raise
Line 163: 
Line 164:         utils.rmFile(P_SYSTEMCTL_CONF)


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:
well, this is ugly... usually these methods of building lists should not change the object status...
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 at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list