Nir Soffer has posted comments on this change. Change subject: supervdsm: generalize udevTrigger method ......................................................................
Patch Set 1: Code-Review-1 (9 comments) This patch should move be done before the previous patch, and even better, move udevTrigger() implementation to udevadm.trigger() https://gerrit.ovirt.org/#/c/44767/1/vdsm/supervdsmServer File vdsm/supervdsmServer: Line 286: def setSafeNetworkConfig(self): Line 287: return setSafeNetworkConfig() Line 288: Line 289: @logDecorator Line 290: def udevTrigger(self, device_name, property_matches=None, device_name is not needed by this method. Line 291: attr_matches=None): Line 292: self.__udevReloadRules(device_name) Line 293: cmd = [EXT_UDEVADM, 'trigger', '--verbose', '--action', 'change'] Line 294: Line 287: return setSafeNetworkConfig() Line 288: Line 289: @logDecorator Line 290: def udevTrigger(self, device_name, property_matches=None, Line 291: attr_matches=None): Better use empty tuple as default value, so we don't need to check later if the parameter is None. Line 292: self.__udevReloadRules(device_name) Line 293: cmd = [EXT_UDEVADM, 'trigger', '--verbose', '--action', 'change'] Line 294: Line 295: if property_matches: Line 288: Line 289: @logDecorator Line 290: def udevTrigger(self, device_name, property_matches=None, Line 291: attr_matches=None): Line 292: self.__udevReloadRules(device_name) device_name is not needed by __udevReloadRules, don't send it. Line 293: cmd = [EXT_UDEVADM, 'trigger', '--verbose', '--action', 'change'] Line 294: Line 295: if property_matches: Line 296: for property_name, property_value in property_matches.items(): Line 291: attr_matches=None): Line 292: self.__udevReloadRules(device_name) Line 293: cmd = [EXT_UDEVADM, 'trigger', '--verbose', '--action', 'change'] Line 294: Line 295: if property_matches: Not needed if default value is a tuple Line 296: for property_name, property_value in property_matches.items(): Line 297: cmd.append('--property-match={}={}'.format(property_name, Line 298: property_value)) Line 299: Line 292: self.__udevReloadRules(device_name) Line 293: cmd = [EXT_UDEVADM, 'trigger', '--verbose', '--action', 'change'] Line 294: Line 295: if property_matches: Line 296: for property_name, property_value in property_matches.items(): Lets use list of tuples instead of a dictionary. This way we don't need to check for None, and the caller control the order of the argument, making the code easier to test, and being better mapping of the underlying command line interface. Also, property_name, property_value are much too verbose in the context of this one line loop. Better use: for name, value in property_matches: cmd.append('--property-match={}={}'.format(name, value)) Line 297: cmd.append('--property-match={}={}'.format(property_name, Line 298: property_value)) Line 299: Line 300: if attr_matches: Line 296: for property_name, property_value in property_matches.items(): Line 297: cmd.append('--property-match={}={}'.format(property_name, Line 298: property_value)) Line 299: Line 300: if attr_matches: Same Line 301: for attr_name, attr_value in attr_matches.items(): Line 302: cmd.append('--attr-match={}={}'.format(attr_name, attr_value)) Line 303: Line 304: self.log.debug("Calling trigger: %s", ' '.join(cmd)) Line 297: cmd.append('--property-match={}={}'.format(property_name, Line 298: property_value)) Line 299: Line 300: if attr_matches: Line 301: for attr_name, attr_value in attr_matches.items(): Same Line 302: cmd.append('--attr-match={}={}'.format(attr_name, attr_value)) Line 303: Line 304: self.log.debug("Calling trigger: %s", ' '.join(cmd)) Line 305: rc, out, err = utils.execCmd(cmd) Line 400: with open(rule_file, "w") as rf: Line 401: self.log.debug("Creating rule %s: %r", rule_file, rule) Line 402: rf.write(rule) Line 403: Line 404: self.udevTrigger('usb_' + '_'.join((bus, device))) Don't call public api from public api. Instead, move udevTrigger() implemetation to udevadm.trigger() Line 405: Line 406: @logDecorator Line 407: def releaseAppropriateUSBDevice(self, bus, device): Line 408: rule_file = _UDEV_RULE_FILE_NAME_USB % (bus, device) Line 415: with open(rule_file, "a") as rf: Line 416: self.log.debug("Creating rule %s: %r", rule_file, rule) Line 417: rf.write(rule) Line 418: Line 419: self.udevTrigger('usb_' + '_'.join((bus, device))) Same as above. Line 420: Line 421: @logDecorator Line 422: def rmAppropriateUSBDevice(self, bus, device): Line 423: self.releaseAppropriateUSBDevice(bus, device) -- To view, visit https://gerrit.ovirt.org/44767 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I761305a54c14e8ac8ef9f4204ee5cc5d55cb31b7 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
