Change in vdsm[master]: hostdev: add udev rules for USB devices

mpolednik at redhat.com mpolednik at redhat.com
Tue Aug 11 19:03:57 UTC 2015


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: 


-- 
To view, visit https://gerrit.ovirt.org/44679
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1f72f63186187254cd4aded0a2e6c396001ca28b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list