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