Change in vdsm[master]: udev: Race fix- load and trigger dev rule(#891300)

abaron at redhat.com abaron at redhat.com
Tue Jan 8 19:11:27 UTC 2013


Ayal Baron has posted comments on this change.

Change subject: udev: Race fix- load and trigger dev rule(#891300)
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 2925:         """
Line 2926:         supervdsm.getProxy().appropriateDevice(guid, thiefId)
Line 2927:         supervdsm.getProxy().udevReloadRules(guid)
Line 2928:         supervdsm.getProxy().udevTrigger(guid)
Line 2929:         supervdsm.getProxy().udevSettle(guid)
this can hang for 3 minutes (udev timeout), I'm not sure that's acceptable.
Line 2930:         try:
Line 2931:             devPath = '/dev/mapper/%s' % guid
Line 2932:             fileUtils.validateQemuReadable(devPath)
Line 2933:         except OSError:


....................................................
File vdsm/supervdsmServer.py
Line 225:     def setSafeNetworkConfig(self):
Line 226:         return configNetwork.setSafeNetworkConfig()
Line 227: 
Line 228:     @logDecorator
Line 229:     def udevReloadRules(self, guid):
why expose 3 low level functions instead of one?
alternatively, if this is in case we want to trigger for multiple devices yet reload and settle once, why pass guid in the API here?

In fact, I'm not sure why all these are not part of appropriateDevice.
The point of supervdsm is to minimize the attack surface, not give generic capabilities to do anything on the system.
Line 230:         cmd = [EXT_UDEVADM, 'control', '--reload-rules']
Line 231:         rc, out, err = misc.execCmd(cmd, sudo=False)
Line 232:         if rc:
Line 233:             raise OSError(errno.EINVAL, "Could not reload-rules for device \


Line 242:             raise OSError(errno.EINVAL, "Could not trigger change for device \
Line 243:                           %s, out %s\nerr %s" % (guid, out, err))
Line 244: 
Line 245:     @logDecorator
Line 246:     def udevSettle(self, guid):
same as above.
Line 247:         cmd = [EXT_UDEVADM, 'settle']
Line 248:         rc, out, err = misc.execCmd(cmd, sudo=False)
Line 249:         if rc:
Line 250:             raise OSError(errno.EINVAL, "Could not settle for device \


--
To view, visit http://gerrit.ovirt.org/10758
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If3b2008a3d9df2dcaf54190721c2dd9764338627
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Lee Yarwood <lyarwood at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list