Shahar Havivi has posted comments on this change. Change subject: v2v: cleanup v2v directory leftovers ......................................................................
Patch Set 2: (8 comments) https://gerrit.ovirt.org/#/c/39789/2/vdsm/v2v.py File vdsm/v2v.py: Line 26: Line 27: from collections import namedtuple Line 28: from contextlib import closing, contextmanager Line 29: import errno Line 30: from glob import glob > let's try to import modules -not names- as much as we can, so Done Line 31: import logging Line 32: import os Line 33: import re Line 34: import threading Line 212: } Line 213: return ret Line 214: Line 215: Line 216: def clean_leftovers(): > Log info "cleaning leftovers" would be nice Done Line 217: for f in glob('%s/*.ovf' % _V2V_DIR): Line 218: try: Line 219: os.remove(f) Line 220: except Exception as e: Line 213: return ret Line 214: Line 215: Line 216: def clean_leftovers(): Line 217: for f in glob('%s/*.ovf' % _V2V_DIR): > Please add log.debug for each file you remove. We should know about every c Done Line 218: try: Line 219: os.remove(f) Line 220: except Exception as e: Line 221: logging.warn('Cannot remove file: %s, err: %s', f, e) Line 213: return ret Line 214: Line 215: Line 216: def clean_leftovers(): Line 217: for f in glob('%s/*.ovf' % _V2V_DIR): > 'f' is a bit too terse Done Line 218: try: Line 219: os.remove(f) Line 220: except Exception as e: Line 221: logging.warn('Cannot remove file: %s, err: %s', f, e) Line 217: for f in glob('%s/*.ovf' % _V2V_DIR): Line 218: try: Line 219: os.remove(f) Line 220: except Exception as e: Line 221: logging.warn('Cannot remove file: %s, err: %s', f, e) > Should be logging.error Done Line 222: Line 223: for f in glob('%s/*.tmp' % _V2V_DIR): Line 224: try: Line 225: os.remove(f) Line 219: os.remove(f) Line 220: except Exception as e: Line 221: logging.warn('Cannot remove file: %s, err: %s', f, e) Line 222: Line 223: for f in glob('%s/*.tmp' % _V2V_DIR): > same, 'f' is too terse. Moreover, better not to recycle identifiers Done Line 224: try: Line 225: os.remove(f) Line 226: except Exception as e: Line 227: logging.warn('Cannot remove file: %s, err: %s', f, e) Line 219: os.remove(f) Line 220: except Exception as e: Line 221: logging.warn('Cannot remove file: %s, err: %s', f, e) Line 222: Line 223: for f in glob('%s/*.tmp' % _V2V_DIR): > We need a warning if we find a stale password file when vdsm starts - this Done Line 224: try: Line 225: os.remove(f) Line 226: except Exception as e: Line 227: logging.warn('Cannot remove file: %s, err: %s', f, e) Line 223: for f in glob('%s/*.tmp' % _V2V_DIR): Line 224: try: Line 225: os.remove(f) Line 226: except Exception as e: Line 227: logging.warn('Cannot remove file: %s, err: %s', f, e) > Should be logging.error Done Line 228: Line 229: Line 230: def _read_ovf(job_id): Line 231: file_name = os.path.join(_V2V_DIR, "%s.ovf" % job_id) -- To view, visit https://gerrit.ovirt.org/39789 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic2f5b63863f4f7dc85d6af0ca015099274715441 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
