Change in vdsm[master]: vm: safer removal of the recovery file
fromani at redhat.com
fromani at redhat.com
Wed Jan 13 10:57:36 UTC 2016
Francesco Romani has posted comments on this change.
Change subject: vm: safer removal of the recovery file
......................................................................
Patch Set 3:
(3 comments)
thanks for pointing out more english mistakes :)
https://gerrit.ovirt.org/#/c/51387/3/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 827: with tempfile.NamedTemporaryFile(dir=constants.P_VDSM_RUN,
Line 828: delete=False) as f:
Line 829: pickle.dump(vmState, f)
Line 830:
Line 831: os.rename(f.name, self._recoveryFile)
> Shouldn't the file writing be inside `with self._recoveryLock' above as wel
To guard against concurrent saveState() calls, sure.
It was made this way on purpose, to guard "only" against concurrent saveState() and _recoveryFile removal.
The problem here is the possibly expensive (even blocking?) I/O operations, we had nasty experiences in the past, but maybe I'm overly cautious here.
Other than that, no reason to have the I/O outside the lock.
Will think about it a bit more.
Line 832: else:
Line 833: self.log.debug('saveState after cleanup for status=%s',
Line 834: self.lastStatus)
Line 835:
Line 1716: def _cleanupRecoveryFile(self):
Line 1717: with self._recoveryLock:
Line 1718: recovery_file = self._recoveryFile
Line 1719: self._recoveryFile = None
Line 1720: if recovery_file:
> We should be consistent with using `is not None' at least within the patch.
good point!
Line 1721: utils.rmFile(self._recoveryFile)
Line 1722:
Line 1723: def _cleanupStatsCache(self):
Line 1724: try:
Line 1717: with self._recoveryLock:
Line 1718: recovery_file = self._recoveryFile
Line 1719: self._recoveryFile = None
Line 1720: if recovery_file:
Line 1721: utils.rmFile(self._recoveryFile)
> Don't forget about this one.
sure, same reason as above why it is outside the lock.
Line 1722:
Line 1723: def _cleanupStatsCache(self):
Line 1724: try:
Line 1725: sampling.stats_cache.remove(self.id)
--
To view, visit https://gerrit.ovirt.org/51387
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a8d2b34599da17bee35c9bb694122ecd9d58068
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani 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: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list