Martin Polednik has posted comments on this change.
Change subject: hostdev: add udev rules for USB devices ......................................................................
Patch Set 3:
(4 comments)
https://gerrit.ovirt.org/#/c/44679/3/vdsm/supervdsmServer File vdsm/supervdsmServer:
Line 296: raise OSError(errno.EINVAL, "Could not trigger change for device \ Line 297: %s, out %s\nerr %s" % (guid, out, err)) Line 298: Line 299: @logDecorator Line 300: def udevTriggerUSB(self, bus, device):
We should not have such apis.
I agree. The tricky part is that we either refactor it first and then create this patch, or create the patch and refactor it later. The first choice is in my opinion a bit safer due to need for 3.6 backport. Line 301: self.__udevReloadRules('usb_' + '_'.join((bus, device))) Line 302: cmd = [EXT_UDEVADM, 'trigger', '--verbose', '--action', 'change', Line 303: '--attr-match=busnum={}'.format(bus), Line 304: '--attr-match=devnum={}'.format(device)]
Line 394: @logDecorator Line 395: def appropriateUSBDevice(self, bus, device): Line 396: rule_file = _UDEV_RULE_FILE_NAME_USB % (bus, device) Line 397: Line 398: rule = ('SUBSYSTEM=="usb", ATTRS{{busnum}}=="{}", '
why not just use string concatenation and/or triple quoted strings?
consistence with other rules Line 399: 'ATTRS{{devnum}}=="{}", ' Line 400: 'OWNER="{}", GROUP="{}"\n').format( Line 401: bus, device, QEMU_PROCESS_USER, QEMU_PROCESS_GROUP) Line 402:
Line 425: def rmAppropriateUSBDevice(self, bus, device): Line 426: self.releaseAppropriateUSBDevice(bus, device) Line 427: # We need to give udev some time to handle the events in queue before Line 428: # deleting the rule. Line 429: udevadm.settle(3)
it exits when things are settled before reaching timeout. Please give a mor
Done Line 430: rule_file = _UDEV_RULE_FILE_NAME_USB % (bus, device) Line 431: Line 432: try: Line 433: os.remove(rule_file)
Line 430: rule_file = _UDEV_RULE_FILE_NAME_USB % (bus, device) Line 431: Line 432: try: Line 433: os.remove(rule_file) Line 434: except:
this looks suspicious, could you please explain a bit?
Too generic, will fix that part. Other then that, it is something I'm not sure we can handle (failure to delete the rule) BUT on the other hand, due to the structure of rule (root->qemu->root) it should be safe to stay in place. Line 435: raise Line 436: else: Line 437: self.log.debug("Removing rule %s", rule_file) Line 438:
vdsm-patches@lists.fedorahosted.org