[PATCH 0/6 V2] Refactore isoinfo.py
From: Aline Manera <alinefm@br.ibm.com> V1 -> V2: - Apply comments made by Ramon and Royce Aline Manera (6): isoinfo: Add default value for ignore_list paramter isoinfo: Use absolute path only for local ISO files Move IsoFormatError() from isoinfo.py to exception.py Move ISO path validation to IsoImage() isoinfo: Move _probe_iso() to IsoImage() pep8 cleanup for isoinfo.py Makefile.am | 1 + src/kimchi/exception.py | 4 ++ src/kimchi/isoinfo.py | 135 +++++++++++++++++++++++----------------------- src/kimchi/model.py | 11 ++-- src/kimchi/scan.py | 5 +- src/kimchi/vmtemplate.py | 8 +-- 6 files changed, 85 insertions(+), 79 deletions(-) -- 1.7.10.4
From: Aline Manera <alinefm@br.ibm.com> ignore_list parameter was added to avoid displaying duplicated results during deep scan. So isoinfo main program must be updated accordingly or it will raise the following error: $ python kimchi/isoinfo.py http://localhost/Fedora-Live-Desktop-x86_64-19-1.iso Traceback (most recent call last): File "kimchi/isoinfo.py", line 343, in <module> probe_iso(None, dict(path=sys.argv[1], updater=updater)) File "kimchi/isoinfo.py", line 283, in probe_iso ignore_list = params['ignore_list'] KeyError: 'ignore_list' Signed-off-by: Aline Manera <alinefm@br.ibm.com> --- src/kimchi/isoinfo.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py index f76fd90..3d57533 100644 --- a/src/kimchi/isoinfo.py +++ b/src/kimchi/isoinfo.py @@ -280,7 +280,7 @@ def probe_iso(status_helper, params): loc = params['path'].encode("utf-8") updater = params['updater'] ignore = False - ignore_list = params['ignore_list'] + ignore_list = params.get('ignore_list', []) def update_result(iso, ret): if ret != ('unknown', 'unknown'): -- 1.7.10.4
From: Aline Manera <alinefm@br.ibm.com> When running isoinfo main program to probe a remote ISO file, it returned a wrong path: $ python kimchi/isoinfo.py http://localhost/Fedora-Live-Desktop-x86_64-19-1.iso [{'path': '/home/alinefm/kimchi/src/http:/localhost/Fedora-Live-Desktop-x86_64-19-1.iso', 'version': '19', 'distro': 'fedora'}] So verify the ISO is a local one and only use abspath() in this case. Otherwise, use the path entried by user. Signed-off-by: Aline Manera <alinefm@br.ibm.com> --- src/kimchi/isoinfo.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py index 3d57533..121241a 100644 --- a/src/kimchi/isoinfo.py +++ b/src/kimchi/isoinfo.py @@ -284,8 +284,8 @@ def probe_iso(status_helper, params): def update_result(iso, ret): if ret != ('unknown', 'unknown'): - iso = os.path.abspath(iso) - updater({'path': iso, 'distro': ret[0], 'version': ret[1]}) + path = os.path.abspath(iso) if os.path.isfile(iso) else iso + updater({'path': path, 'distro': ret[0], 'version': ret[1]}) if os.path.isdir(loc): for root, dirs, files in os.walk(loc): -- 1.7.10.4
From: Aline Manera <alinefm@br.ibm.com> All Kimchi exception should be in exception.py So move IsoFormatError() to there and update imports accordingly. Signed-off-by: Aline Manera <alinefm@br.ibm.com> --- src/kimchi/exception.py | 4 ++++ src/kimchi/isoinfo.py | 5 +---- src/kimchi/model.py | 6 +++--- src/kimchi/vmtemplate.py | 4 ++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/kimchi/exception.py b/src/kimchi/exception.py index d7a2835..8143b05 100644 --- a/src/kimchi/exception.py +++ b/src/kimchi/exception.py @@ -34,3 +34,7 @@ class InvalidParameter(Exception): class InvalidOperation(Exception): pass + + +class IsoFormatError(Exception): + pass diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py index 121241a..7d919a0 100644 --- a/src/kimchi/isoinfo.py +++ b/src/kimchi/isoinfo.py @@ -28,6 +28,7 @@ import sys import urllib2 +from kimchi.exception import IsoFormatError from kimchi.utils import kimchi_log iso_dir = [ @@ -117,10 +118,6 @@ iso_dir = [ ] -class IsoFormatError(Exception): - pass - - class IsoImage(object): """ Scan an iso9660 image to extract the Volume ID and check for boot-ability diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a6790b8..decb889 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -62,8 +62,8 @@ from kimchi import vnc from kimchi import xmlutils from kimchi.asynctask import AsyncTask from kimchi.distroloader import DistroLoader -from kimchi.exception import InvalidOperation, InvalidParameter, MissingParameter -from kimchi.exception import NotFoundError, OperationFailed +from kimchi.exception import InvalidOperation, InvalidParameter, IsoFormatError +from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.featuretests import FeatureTests from kimchi.networkxml import to_network_xml from kimchi.objectstore import ObjectStore @@ -1167,7 +1167,7 @@ class Model(object): try: os_distro, os_version = isoinfo.probe_one(path) bootable = True - except isoinfo.IsoFormatError: + except IsoFormatError: bootable = False res.update( dict(os_distro=os_distro, os_version=os_version, path=path, bootable=bootable)) diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index dd43faa..8bf3df9 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -29,7 +29,7 @@ import urlparse from kimchi import isoinfo from kimchi import osinfo -from kimchi.exception import InvalidParameter +from kimchi.exception import InvalidParameter, IsoFormatError QEMU_NAMESPACE = "xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'" @@ -64,7 +64,7 @@ class VMTemplate(object): try: iso_distro, iso_version = isoinfo.probe_one(iso) - except isoinfo.IsoFormatError, e: + except IsoFormatError, e: raise InvalidParameter(e) # Fetch defaults based on the os distro and version -- 1.7.10.4
From: Aline Manera <alinefm@br.ibm.com> That way we will have a single point to check ISO path validation. And the user doesn't need to care about it as IsoImage() will check if the ISO path is local, remote or invalid. Signed-off-by: Aline Manera <alinefm@br.ibm.com> --- src/kimchi/isoinfo.py | 52 ++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py index 7d919a0..d605ee2 100644 --- a/src/kimchi/isoinfo.py +++ b/src/kimchi/isoinfo.py @@ -134,13 +134,36 @@ class IsoImage(object): EL_TORITO_VALIDATION_ENTRY = struct.Struct("=BBH24sHBB") EL_TORITO_BOOT_ENTRY = struct.Struct("=BBHBBHL20x") - def __init__(self, path, remote = False): + def __init__(self, path, remote = None): self.path = path self.volume_id = None self.bootable = False + self.remote = remote + if self.remote is None: + self.remote = self._is_iso_remote() + self._scan() + def _is_iso_remote(self): + if os.path.isfile(self.path): + return False + + if self._check_url_path(): + return True + + raise IsoFormatError('ISO %s does not exist' % self.path) + + def _check_url_path(self): + try: + code = urllib2.urlopen(self.path).getcode() + if code != 200: + return False + except (urllib2.HTTPError, ValueError): + return False + + return True + def _unpack(self, s, data): return s.unpack(data[:s.size]) @@ -246,7 +269,7 @@ class Matcher(object): return self.lastmatch.group(num) -def _probe_iso(fname, remote = False): +def _probe_iso(fname, remote = None): try: iso = IsoImage(fname, remote) except Exception, e: @@ -298,39 +321,20 @@ def probe_iso(status_helper, params): continue iso = os.path.join(root, name) try: - ret = _probe_iso(iso) + ret = _probe_iso(iso, False) update_result(iso, ret) except: continue - elif os.path.isfile(loc): - ret = _probe_iso(loc, False) - update_result(loc, ret) else: - ret = _probe_iso(loc, True) + ret = _probe_iso(loc) update_result(loc, ret) if status_helper != None: status_helper('', True) -def _check_url_path(path): - try: - code = urllib2.urlopen(path).getcode() - if code != 200: - return False - except (urllib2.HTTPError, ValueError): - return False - - return True def probe_one(iso): - if os.path.isfile(iso): - remote = False - elif _check_url_path(iso): - remote = True - else: - raise IsoFormatError('ISO %s does not exist' % iso) - - return _probe_iso(iso, remote) + return _probe_iso(iso) if __name__ == '__main__': -- 1.7.10.4
From: Aline Manera <alinefm@br.ibm.com> That way all operations related to IsoImage() object will be in this class. Signed-off-by: Aline Manera <alinefm@br.ibm.com> --- src/kimchi/isoinfo.py | 57 +++++++++++++++++++--------------------------- src/kimchi/model.py | 5 ++-- src/kimchi/scan.py | 5 ++-- src/kimchi/vmtemplate.py | 4 +++- 4 files changed, 33 insertions(+), 38 deletions(-) diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py index d605ee2..e2cdbeb 100644 --- a/src/kimchi/isoinfo.py +++ b/src/kimchi/isoinfo.py @@ -164,6 +164,26 @@ class IsoImage(object): return True + def probe(self): + if not self.bootable: + raise IsoFormatError("ISO %s not bootable" % self.path) + + matcher = Matcher(self.volume_id) + + for d, v, regex in iso_dir: + if matcher.search(regex): + distro = d + if hasattr(v, '__call__'): + version = v(matcher) + else: + version = v + return (distro, version) + + kimchi_log.debug("probe_iso: Unable to identify ISO %s with Volume ID: %s" + % (self.path, self.volume_id)) + + return ('unknown', 'unknown') + def _unpack(self, s, data): return s.unpack(data[:s.size]) @@ -269,33 +289,6 @@ class Matcher(object): return self.lastmatch.group(num) -def _probe_iso(fname, remote = None): - try: - iso = IsoImage(fname, remote) - except Exception, e: - kimchi_log.warning("probe_iso: Error processing ISO image: %s\n%s" % - (fname, e)) - raise IsoFormatError(e) - - if not iso.bootable: - raise IsoFormatError("ISO %s not bootable" % fname) - - matcher = Matcher(iso.volume_id) - - for d, v, regex in iso_dir: - if matcher.search(regex): - distro = d - if hasattr(v, '__call__'): - version = v(matcher) - else: - version = v - return (distro, version) - - kimchi_log.debug("probe_iso: Unable to identify ISO %s with Volume ID: %s" - % (fname, iso.volume_id)) - return ('unknown', 'unknown') - - def probe_iso(status_helper, params): loc = params['path'].encode("utf-8") updater = params['updater'] @@ -321,22 +314,20 @@ def probe_iso(status_helper, params): continue iso = os.path.join(root, name) try: - ret = _probe_iso(iso, False) + iso_img = IsoImage(iso) + ret = iso_img.probe() update_result(iso, ret) except: continue else: - ret = _probe_iso(loc) + iso_img = IsoImage(loc) + ret = iso_img.probe() update_result(loc, ret) if status_helper != None: status_helper('', True) -def probe_one(iso): - return _probe_iso(iso) - - if __name__ == '__main__': iso_list = [] def updater(iso_info): diff --git a/src/kimchi/model.py b/src/kimchi/model.py index decb889..3547b73 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -55,7 +55,6 @@ except ImportError: from kimchi import config -from kimchi import isoinfo from kimchi import netinfo from kimchi import network as knetwork from kimchi import vnc @@ -65,6 +64,7 @@ from kimchi.distroloader import DistroLoader from kimchi.exception import InvalidOperation, InvalidParameter, IsoFormatError from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.featuretests import FeatureTests +from kimchi.isoinfo import IsoImage from kimchi.networkxml import to_network_xml from kimchi.objectstore import ObjectStore from kimchi.scan import Scanner @@ -1165,7 +1165,8 @@ class Model(object): path = os.path.join(os.path.dirname(path), os.readlink(path)) os_distro = os_version = 'unknown' try: - os_distro, os_version = isoinfo.probe_one(path) + iso_img = IsoImage(path) + os_distro, os_version = iso_img.probe() bootable = True except IsoFormatError: bootable = False diff --git a/src/kimchi/scan.py b/src/kimchi/scan.py index 27be1da..e192f01 100644 --- a/src/kimchi/scan.py +++ b/src/kimchi/scan.py @@ -29,7 +29,7 @@ import tempfile import time -from kimchi.isoinfo import probe_iso, probe_one +from kimchi.isoinfo import IsoImage, probe_iso from kimchi.utils import kimchi_log @@ -73,7 +73,8 @@ class Scanner(object): duplicates = "%s/%s*" % (params['pool_path'], iso_name) for f in glob.glob(duplicates): - if (iso_info['distro'], iso_info['version']) == probe_one(f): + iso_img = IsoImage(f) + if (iso_info['distro'], iso_info['version']) == iso_img.probe(): return iso_path = iso_name + hashlib.md5(iso_info['path']).hexdigest() + '.iso' diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py index 8bf3df9..12da0d1 100644 --- a/src/kimchi/vmtemplate.py +++ b/src/kimchi/vmtemplate.py @@ -30,6 +30,7 @@ import urlparse from kimchi import isoinfo from kimchi import osinfo from kimchi.exception import InvalidParameter, IsoFormatError +from kimchi.isoinfo import IsoImage QEMU_NAMESPACE = "xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'" @@ -63,7 +64,8 @@ class VMTemplate(object): self.info.update({'iso_stream': True}) try: - iso_distro, iso_version = isoinfo.probe_one(iso) + iso_img = IsoImage(iso) + iso_distro, iso_version = iso_img.probe() except IsoFormatError, e: raise InvalidParameter(e) -- 1.7.10.4
From: Aline Manera <alinefm@br.ibm.com> This patch cleans up pep8 style issue in isoinfo.py Signed-off-by: Aline Manera <alinefm@br.ibm.com> --- Makefile.am | 1 + src/kimchi/isoinfo.py | 29 +++++++++++++++++------------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/Makefile.am b/Makefile.am index 1fb3502..2e354e1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -47,6 +47,7 @@ PEP8_WHITELIST = \ src/kimchi/config.py.in \ src/kimchi/disks.py \ src/kimchi/featuretests.py \ + src/kimchi/isoinfo.py \ src/kimchi/rollbackcontext.py \ src/kimchi/root.py \ src/kimchi/server.py \ diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py index e2cdbeb..4b9cef7 100644 --- a/src/kimchi/isoinfo.py +++ b/src/kimchi/isoinfo.py @@ -18,7 +18,7 @@ # # You should have received a copy of the GNU Lesser General Public # License along with this library; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA import glob import os @@ -31,6 +31,7 @@ import urllib2 from kimchi.exception import IsoFormatError from kimchi.utils import kimchi_log + iso_dir = [ ## # Portions of this data from libosinfo: http://libosinfo.org/ @@ -134,7 +135,7 @@ class IsoImage(object): EL_TORITO_VALIDATION_ENTRY = struct.Struct("=BBH24sHBB") EL_TORITO_BOOT_ENTRY = struct.Struct("=BBHBBHL20x") - def __init__(self, path, remote = None): + def __init__(self, path, remote=None): self.path = path self.volume_id = None self.bootable = False @@ -179,8 +180,8 @@ class IsoImage(object): version = v return (distro, version) - kimchi_log.debug("probe_iso: Unable to identify ISO %s with Volume ID: %s" - % (self.path, self.volume_id)) + msg = "probe_iso: Unable to identify ISO %s with Volume ID: %s" + kimchi_log.debug(msg % (self.path, self.volume_id)) return ('unknown', 'unknown') @@ -191,9 +192,9 @@ class IsoImage(object): """ Search the Volume Descriptor Table for an El Torito boot record. If found, the boot record will provide a link to a boot catalogue. The - first entry in the boot catalogue is a validation entry. The next entry - contains the default boot entry. The default boot entry will indicate - whether the image is considered bootable. + first entry in the boot catalogue is a validation entry. The next + entry contains the default boot entry. The default boot entry will + indicate whether the image is considered bootable. """ vd_type = -1 for i in xrange(1, 4): @@ -213,7 +214,8 @@ class IsoImage(object): raise IsoFormatError("Invalid El Torito boot record") offset = IsoImage.SECTOR_SIZE * boot_cat - size = IsoImage.EL_TORITO_VALIDATION_ENTRY.size + IsoImage.EL_TORITO_BOOT_ENTRY.size + size = IsoImage.EL_TORITO_VALIDATION_ENTRY.size + \ + IsoImage.EL_TORITO_BOOT_ENTRY.size data = self._get_iso_data(offset, size) fmt = IsoImage.EL_TORITO_VALIDATION_ENTRY @@ -241,8 +243,8 @@ class IsoImage(object): Volume ID from the table """ primary_vol_data = data[0: -1] - (vd_type, vd_ident, vd_ver, - pad0, sys_id, vol_id) = self._unpack(IsoImage.VOL_DESC, primary_vol_data) + info = self._unpack(IsoImage.VOL_DESC, primary_vol_data) + (vd_type, vd_ident, vd_ver, pad0, sys_id, vol_id) = info if vd_type != 1: raise IsoFormatError("Unexpected volume type for primary volume") if vd_ident != 'CD001' or vd_ver != 1: @@ -252,7 +254,8 @@ class IsoImage(object): def _get_iso_data(self, offset, size): if self.remote: request = urllib2.Request(self.path) - request.add_header("range", "bytes=%d-%d" % (offset, offset + size -1)) + range_header = "bytes=%d-%d" % (offset, offset + size - 1) + request.add_header("range", range_header) response = urllib2.urlopen(request) data = response.read() else: @@ -324,13 +327,15 @@ def probe_iso(status_helper, params): ret = iso_img.probe() update_result(loc, ret) - if status_helper != None: + if status_helper is not None: status_helper('', True) if __name__ == '__main__': iso_list = [] + def updater(iso_info): iso_list.append(iso_info) + probe_iso(None, dict(path=sys.argv[1], updater=updater)) print iso_list -- 1.7.10.4
participants (1)
-
Aline Manera