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@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server