Change in vdsm[master]: v2v: ova should support zip and extracted directory formats
fromani at redhat.com
fromani at redhat.com
Thu Nov 5 13:06:27 UTC 2015
Francesco Romani has posted comments on this change.
Change subject: v2v: ova should support zip and extracted directory formats
......................................................................
Patch Set 1: Code-Review-1
(8 comments)
few questions and comments inside.
https://gerrit.ovirt.org/#/c/48129/1/vdsm/v2v.py
File vdsm/v2v.py:
Line 744: if os.path.isdir(ova_path):
Line 745: return OVA_FORMAT.DIR
Line 746: elif zipfile.is_zipfile(ova_path):
Line 747: return OVA_FORMAT.ZIP
Line 748: return OVA_FORMAT.TAR
not sure we should default on TAR.
I'd check if it is indeed tar, and raise otherwise. But perhaps could be done into a later patch, or may not be a problem at all.
What do you think?
Line 749:
Line 750:
Line 751: def _read_ovf_from_ova(ova_path):
Line 752: '''
Line 752: '''
Line 753: virt-v2v support ova in tar, zip formats as well as
Line 754: extracted directory
Line 755: '''
Line 756: ova_format = _get_ova_file_type(ova_path)
I don't think we need this helper after all, we can inline the code here.
Line 757: if ova_format == OVA_FORMAT.DIR:
Line 758: return _read_ovf_from_ova_dir(ova_path)
Line 759: elif ova_format == OVA_FORMAT.TAR:
Line 760: return _read_ovf_from_tar_ova(ova_path)
Line 763:
Line 764:
Line 765: def _read_ovf_from_ova_dir(ova_path):
Line 766: files = os.listdir(ova_path)
Line 767: for f in files:
caution: down below you are shadowing this variable with 'f' being a file object. This can lead to nasty bugs. I suggest you to rename the loop variable 'ova_file'.
Line 768: if '.ovf' in f.lower():
Line 769: f = open(os.path.join(ova_path, f), 'r')
Line 770: with closing(f):
Line 771: return ''.join(f.readlines())
Line 764:
Line 765: def _read_ovf_from_ova_dir(ova_path):
Line 766: files = os.listdir(ova_path)
Line 767: for f in files:
Line 768: if '.ovf' in f.lower():
it is HIGHLY unlikely that we could encounter something like
'blah.ovf.bin', but this check will needlessly pass.
I suggest something like
if f.lower().endswith('.ovf'):
# ...
please check if the os.path module has a better tool to offer.
Line 769: f = open(os.path.join(ova_path, f), 'r')
Line 770: with closing(f):
Line 771: return ''.join(f.readlines())
Line 772: raise ClientError('OVA directory %s does not contain ovf file' % ova_path)
Line 766: files = os.listdir(ova_path)
Line 767: for f in files:
Line 768: if '.ovf' in f.lower():
Line 769: f = open(os.path.join(ova_path, f), 'r')
Line 770: with closing(f):
this is good practice in general, but it is not needed for plain files.
It is perfectly fine and safe to just have
with open(os.path.join(ova_path, ova_file), 'r') as f:
...
Line 771: return ''.join(f.readlines())
Line 772: raise ClientError('OVA directory %s does not contain ovf file' % ova_path)
Line 773:
Line 774:
Line 767: for f in files:
Line 768: if '.ovf' in f.lower():
Line 769: f = open(os.path.join(ova_path, f), 'r')
Line 770: with closing(f):
Line 771: return ''.join(f.readlines())
return f.read()
seems simpler and faster
Line 772: raise ClientError('OVA directory %s does not contain ovf file' % ova_path)
Line 773:
Line 774:
Line 775: def _read_ovf_from_zip_ova(ova_path):
Line 773:
Line 774:
Line 775: def _read_ovf_from_zip_ova(ova_path):
Line 776: fh = file(ova_path, 'rb')
Line 777: with closing(fh):
same as per line 770. Plus, it is recommended to use 'open' and not 'file'.
Line 778: zf = zipfile.ZipFile(fh)
Line 779: for name in zf.namelist():
Line 780: if '.ovf' in name:
Line 781: return zf.read(name)
Line 776: fh = file(ova_path, 'rb')
Line 777: with closing(fh):
Line 778: zf = zipfile.ZipFile(fh)
Line 779: for name in zf.namelist():
Line 780: if '.ovf' in name:
same as per line 768
Line 781: return zf.read(name)
Line 782: raise ClientError('OVA does not contains file with .ovf suffix')
Line 783:
Line 784:
--
To view, visit https://gerrit.ovirt.org/48129
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I494b88709c4c23fd39690b589eff1134e74f81ba
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list