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

nsoffer at redhat.com nsoffer at redhat.com
Wed Aug 12 15:10:49 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: hostdev: add udev rules for USB devices
......................................................................


Patch Set 3:

(5 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):
> Please see the followup - I believe that udevadm things belong to udevadm m
I agree that udevadm stuff belongs to udevadm, this is why we should not add  such code here.

I don't want to make this code worse, and support this code during 3.6 lifetime. We should make this change in a clean way, and backport all the required patches to 3.6.
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 391:         if not error:
Line 392:             self.udevTrigger(iommu_group)
Line 393: 
Line 394:     @logDecorator
Line 395:     def appropriateUSBDevice(self, bus, device):
If you want to add new api with more specific name, you should change the old api to use also a more specific name, like appropriateMultipathDevice.
Line 396:         rule_file = _UDEV_RULE_FILE_NAME_USB % (bus, device)
Line 397: 
Line 398:         rule = ('SUBSYSTEM=="usb", ATTRS{{busnum}}=="{}", '
Line 399:                 'ATTRS{{devnum}}=="{}", '


Line 401:                     bus, device, QEMU_PROCESS_USER, QEMU_PROCESS_GROUP)
Line 402: 
Line 403:         with open(rule_file, "w") as rf:
Line 404:             self.log.debug("Creating rule %s: %r", rule_file, rule)
Line 405:             rf.write(rule)
This part is duplicated too many times. Please move the implemenation to new udev module - udev.add_rule()
Line 406: 
Line 407:         self.udevTriggerUSB(bus, device)
Line 408: 
Line 409:     @logDecorator


Line 406: 
Line 407:         self.udevTriggerUSB(bus, device)
Line 408: 
Line 409:     @logDecorator
Line 410:     def releaseAppropriateUSBDevice(self, bus, device):
Does not make sense.
Line 411:         rule_file = _UDEV_RULE_FILE_NAME_USB % (bus, device)
Line 412: 
Line 413:         rule = ('SUBSYSTEM=="usb", ATTRS{{busnum}}=="{}", '
Line 414:                 'ATTRS{{devnum}}=="{}", '


Line 422:         self.udevTriggerUSB(bus, device)
Line 423: 
Line 424:     @logDecorator
Line 425:     def rmAppropriateUSBDevice(self, bus, device):
Line 426:         self.releaseAppropriateUSBDevice(bus, device)
Why do you need this?

Removing a rule should do:

    remove the rule
    reload udev rules

udev will apply the default rules as needed, no need to change the device permissions and wait for completion.

If you want to change the permissions without waiting for udev, you can do:

    remove the rule
    change permissions using os.chown
    reload rules

Please explain why this will not work for your use case.
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)
Line 430:         rule_file = _UDEV_RULE_FILE_NAME_USB % (bus, device)


-- 
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