Nir Soffer has posted comments on this change.
Change subject: supervdsm: move udev functions to udevadm module ......................................................................
Patch Set 1: Code-Review-1
(10 comments)
Thanks for this cleanup!
https://gerrit.ovirt.org/#/c/44684/1/lib/vdsm/udevadm.py File lib/vdsm/udevadm.py:
Line 23: import errno Line 24: Line 25: from . import utils Line 26: Line 27: from vdsm.constants import EXT_UDEVADM Why do you need EXT_UDEVADM, when this module is finding udevadm via CommandPath? Line 28: Line 29: _UDEVADM = utils.CommandPath("udevadm", "/sbin/udevadm", "/usr/sbin/udevadm") Line 30: _UDEV_WITH_RELOAD_VERSION = 181 Line 31:
Line 26: Line 27: from vdsm.constants import EXT_UDEVADM Line 28: Line 29: _UDEVADM = utils.CommandPath("udevadm", "/sbin/udevadm", "/usr/sbin/udevadm") Line 30: _UDEV_WITH_RELOAD_VERSION = 181 Unneeded Line 31: Line 32: Line 33: class Error(Exception): Line 34:
Line 66: logging.error("%s", e) Line 67: Line 68: Line 69: def has_reload(): Line 70: return version() > _UDEV_WITH_RELOAD_VERSION This is not needed any more, we support only el7 (version 208) and fedora >= 21 (version 216). Line 71: Line 72: Line 73: def reload_rules(device_name): Line 74: if has_reload():
Line 69: def has_reload(): Line 70: return version() > _UDEV_WITH_RELOAD_VERSION Line 71: Line 72: Line 73: def reload_rules(device_name): device_name is unused, we should not pass unneeded parameters, to add context to the error. The caller should log this properly.
Rename to reload(), as recent udevadm uses this term. Line 74: if has_reload(): Line 75: reload = "--reload" Line 76: else: Line 77: reload = "--reload-rules"
Line 73: def reload_rules(device_name): Line 74: if has_reload(): Line 75: reload = "--reload" Line 76: else: Line 77: reload = "--reload-rules" we always have reload Line 78: cmd = [EXT_UDEVADM, 'control', reload] Line 79: rc, out, err = utils.execCmd(cmd) Line 80: if rc: Line 81: raise OSError(errno.EINVAL, "Could not reload-rules for device "
Line 78: cmd = [EXT_UDEVADM, 'control', reload] Line 79: rc, out, err = utils.execCmd(cmd) Line 80: if rc: Line 81: raise OSError(errno.EINVAL, "Could not reload-rules for device " Line 82: "%s" % device_name) Use _run_command()
Never fake OSError. We have bad code doing this but we should fix that code, not add new evil errors. Line 83: Line 84: Line 85: @utils.memoized Line 86: def version():
Line 87: cmd = [EXT_UDEVADM, '--version'] Line 88: rc, out, err = utils.execCmd(cmd) Line 89: if rc: Line 90: raise RuntimeError("Could not get udev version number") Line 91: return int(out[0]) Do we need this after removing has_reload?
If this is needed only for infrequent use, don't memoize it so it continue to work after upgrading udevadm.
If keeping this, raise Error(). Don't use RuntimeError does not help any one. Line 92: Line 93: Line 94: def _run_command(args): Line 95: cmd = [_UDEVADM.cmd]
Line 98: if rc != 0: Line 99: raise Error(rc, out, err) Line 100: Line 101: Line 102: def udev_trigger(guid): Rename to trigger(), make the interface generic so we can use it for all type of calls. Line 103: reload_rules(guid) Line 104: cmd = [EXT_UDEVADM, 'trigger', '--verbose', '--action', 'change', Line 105: '--property-match=DM_NAME=%s' % guid] Line 106: rc, out, err = utils.execCmd(cmd)
Line 105: '--property-match=DM_NAME=%s' % guid] Line 106: rc, out, err = utils.execCmd(cmd) Line 107: if rc: Line 108: raise OSError(errno.EINVAL, "Could not trigger change for device \ Line 109: %s, out %s\nerr %s" % (guid, out, err)) Use _run_command()
Fix the callers to handle udevadm.Error instead of this fake OSError. Line 110: Line 111: Line 112: def udev_trigger_usb(bus, device): Line 113: reload_rules('usb_' + '_'.join((bus, device)))
Line 108: raise OSError(errno.EINVAL, "Could not trigger change for device \ Line 109: %s, out %s\nerr %s" % (guid, out, err)) Line 110: Line 111: Line 112: def udev_trigger_usb(bus, device): Unneeded, trigger() should be good enough to do what you need. Line 113: reload_rules('usb_' + '_'.join((bus, device))) Line 114: cmd = [EXT_UDEVADM, 'trigger', '--verbose', '--action', 'change', Line 115: '--attr-match=busnum={}'.format(bus), Line 116: '--attr-match=devnum={}'.format(device)]