Dan Kenigsberg has posted comments on this change.
Change subject: gluster: VDSM Gluster verbs to manage UFO/swift configuration
......................................................................
Patch Set 12: I would prefer that you didn't submit this
(7 inline comments)
....................................................
File vdsm_cli/vdsClientGluster.py
Line 295: raise ValueError
Line 296: configDictStr = params.get('configDict', '{}')
Line 297:
Line 298: try:
Line 299: configDict = json.loads(configDictStr)
ast.literal_eval() may be more permissive to spaces/quotes combinations.
Line 300: except:
Line 301: raise ValueError
Line 302:
Line 303: status = self.s.glusterSwiftConfigSet(serverType, configDict)
....................................................
File vdsm/gluster/cli.py
Line 911:
Line 912: config_file = SWIFT_CONFIG_FILES[serverType]
Line 913: try:
Line 914: config.readfp(open(config_file))
Line 915: except IOError as e:
IOError may be about open, but also other errno. Please log or (better)
propagate e.errno.
Line 916: raise
ge.GlusterSwiftConfigOpenFailedException(err=[e.strerror])
Line 917:
Line 918: return config
Line 919:
Line 920:
Line 921: def _writeSwiftConfigFile(serverType, config):
Line 922: config_file = SWIFT_CONFIG_FILES[serverType]
Line 923: try:
Line 924: with open(config_file, 'wb') as configFile:
that's not safe in case vdsm dies while it has written part of the configFile.
You should do the write/rename dance..
Line 925: config.write(configFile)
Line 926: except IOError as e:
Line 927: raise
ge.GlusterSwiftConfigWriteFailedException(err=[e.strerror])
Line 928:
Line 951: # default options in every section
Line 952: defaultValues = dict(config.defaults())
Line 953: config._defaults = {}
Line 954:
Line 955: if section == 'DEFAULT':
better use ConfigParser.DEFAULTSECT
Line 956: configValues['DEFAULT'] = _getConfigOptions(defaultValues,
Line 957: configOption)
Line 958: elif section:
Line 959: if not config.has_section(section):
Line 959: if not config.has_section(section):
Line 960: errMsg = "%s: Invalid Swift Config Section" % section
Line 961: raise
ge.GlusterSwiftConfigInvalidSectionException(err=[errMsg])
Line 962:
Line 963: items = dict(config.items(section))
why do you cast this to a new dict()?
Line 964: configValues[section] = _getConfigOptions(items, configOption)
Line 965: else:
Line 966: sections = config.sections()
Line 967: configValues['DEFAULT'] = defaultValues
Line 987: defaultValues = dict(config.defaults())
Line 988: config._defaults = {}
Line 989:
Line 990: for section in configDict:
Line 991: if not config.has_section(section) and section.upper() !=
"DEFAULT":
Why do you disallow sections named "DeFAult"?
They are stupid, but I don't think it is our business to forbid them.
Line 992: config.add_section(section)
Line 993:
Line 994: for configOption in configDict[section]:
Line 995: value = configDict[section][configOption]
Line 997: # update defaultValues dict instead of config object
Line 998: if section == "DEFAULT":
Line 999: defaultValues[configOption] = value
Line 1000: else:
Line 1001: config.set(section, configOption, str(value))
str() explodes on unicode characters.
Line 1002:
Line 1003: # Restore defaults
Line 1004: config._defaults = defaultValues
--
To view, visit http://gerrit.ovirt.org/10864
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie966fb515275a0768f67cbbe2055a07002355327
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Aravinda VK <avish...@redhat.com>
Gerrit-Reviewer: Aravinda VK <avish...@redhat.com>
Gerrit-Reviewer: Bala.FA <barum...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
