[node-patches] Change in ovirt-node[master]: DRAFT: RFC: Incomplete version of tuned configuration

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: DRAFT: RFC: Incomplete version of tuned configuration
......................................................................


Patch Set 1: (10 inline comments)

....................................................
File src/ovirt/node/setup/performance_page.py
Line 47:     def validators(self):
Line 48:         return {}
Line 49: 
Line 50:     def ui_content(self):
Line 51:         profiles = [(profile,profile) for profile in tuned.get_available_profiles()]
A missing whitespace after the comma
Line 52:         # Include an option to disable tuned
Line 53:         profiles.append(("None","None"))
Line 54: 
Line 55:         ws = [ui.Header("header", "tuned Configuration"),


Line 49: 
Line 50:     def ui_content(self):
Line 51:         profiles = [(profile,profile) for profile in tuned.get_available_profiles()]
Line 52:         # Include an option to disable tuned
Line 53:         profiles.append(("None","None"))
a missing whitespace after the comma, and maybe the name of the entry should be "Disable" ?
Line 54: 
Line 55:         ws = [ui.Header("header", "tuned Configuration"),
Line 56:               ui.Label("label", "Choose the tuned profile you would " +
Line 57:                        "like to apply to this system."),


Line 56:               ui.Label("label", "Choose the tuned profile you would " +
Line 57:                        "like to apply to this system."),
Line 58:               ui.Divider("divider[0]"),
Line 59:               ui.KeywordLabel("tuned.profile", "Current Active Profile:  "),
Line 60:               ui.Table("tuned.profile", "", "Available tuned Profiles",
The first argument is the name of the widget. The KeywordLabel and the Table got the same names.
Please use different names for different widgets, e.g. tuned.profiles for the Table.
Line 61:                        profiles),
Line 62:               ]
Line 63:         page = ui.Page("page", ws)
Line 64:         self.widgets.add(page)


....................................................
File src/ovirt/node/utils/tuned.py
Line 59:         A boolean based on the return of tuned-adm
Line 60:     """
Line 61:     try:
Line 62:         if ( profile == "None" or profile = "off" ):
Line 63:             ret = process.check_call("/usr/sbin/tuned-adm off")
check_call() will raise an exception if retval != 0
Line 64:             if ret == 0:
Line 65:                 return True
Line 66:             else:
Line 67:                 return False


Line 61:     try:
Line 62:         if ( profile == "None" or profile = "off" ):
Line 63:             ret = process.check_call("/usr/sbin/tuned-adm off")
Line 64:             if ret == 0:
Line 65:                 return True
Please try to only use one return statement - at the end of the method.
This can be accomplished by using exceptions ion places where you'd typically return False
Line 66:             else:
Line 67:                 return False
Line 68:         elif profile not in get_available_profiles()
Line 69:             # error


Line 63:             ret = process.check_call("/usr/sbin/tuned-adm off")
Line 64:             if ret == 0:
Line 65:                 return True
Line 66:             else:
Line 67:                 return False
Please use exceptions!
Line 68:         elif profile not in get_available_profiles()
Line 69:             # error
Line 70:             #FIXME:  log some message
Line 71:             return False


Line 67:                 return False
Line 68:         elif profile not in get_available_profiles()
Line 69:             # error
Line 70:             #FIXME:  log some message
Line 71:             return False
No, please use execptions!
E.g. RuntimeError(msg)
Line 72:         else:
Line 73:             ret = process.check_call("/usr/sbin/tuned-adm profile %s" % profile)
Line 74:             if ret == 0:
Line 75:                 return True


Line 69:             # error
Line 70:             #FIXME:  log some message
Line 71:             return False
Line 72:         else:
Line 73:             ret = process.check_call("/usr/sbin/tuned-adm profile %s" % profile)
check_call will raise an exception when retval != 0
Line 74:             if ret == 0:
Line 75:                 return True
Line 76:             else:
Line 77:                 return False


Line 71:             return False
Line 72:         else:
Line 73:             ret = process.check_call("/usr/sbin/tuned-adm profile %s" % profile)
Line 74:             if ret == 0:
Line 75:                 return True
Please use exceptions!
Line 76:             else:
Line 77:                 return False
Line 78:     except:


Line 75:                 return True
Line 76:             else:
Line 77:                 return False
Line 78:     except:
Line 79:         return False
Please use exceptions!


--
To view, visit http://gerrit.ovirt.org/14962
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1ed6db48bfeeed962f1bff656d71243281e1597
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Michael Burns <mburns at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>