Change in vdsm[master]: gluster: VDSM Gluster verbs to manage UFO/swift configuratio...

[Dan Kenigsberg]

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 
Gerrit-Reviewer: Aravinda VK 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Shireesh Anjal 
Gerrit-Reviewer: oVirt Jenkins CI Server