Nir Soffer has posted comments on this change.

Change subject: supervdsm: move udevadm methods to udevadm module
......................................................................


Patch Set 3: Code-Review-1

(2 comments)

https://gerrit.ovirt.org/#/c/44808/3/lib/vdsm/udevadm.py
File lib/vdsm/udevadm.py:

Line 84: 
Line 85:                         and causes only events from devices that match
Line 86:                         given property to be triggered.
Line 87:     '''
Line 88:     self.__udevReloadRules()
> this should be renamed or dropped, propably
__udevReloadRules should move here.
Line 89:     cmd = ['trigger', '--verbose', '--action', 'change']
Line 90: 
Line 91:     for name, value in property_matches:
Line 92:         cmd.append('--property-match={}={}'.format(name, value))


https://gerrit.ovirt.org/#/c/44808/3/vdsm/supervdsmServer
File vdsm/supervdsmServer:

Line 281:     def setSafeNetworkConfig(self):
Line 282:         return setSafeNetworkConfig()
Line 283: 
Line 284:     @logDecorator
Line 285:     def udevTrigger(self, *args, **kwargs):
Using *args, **kwargs make it hard to understand the code. Now you have to 
drill down to udevadm.trigger to understand what are the arguments.

Should be:

    def udevTrigger(self, attr_matches=(), property_matches=()):
Line 286:         try:
Line 287:             udevadm.trigger(*args, **kwargs)
Line 288:         except udevadm.Error as e:
Line 289:             raise OSError(errno.EINVAL, 'Could not trigger change '


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2b72b580e77872068ae6357410d940a83626b05
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <[email protected]>
Gerrit-Reviewer: Francesco Romani <[email protected]>
Gerrit-Reviewer: Jenkins CI
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

Reply via email to