Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 01/06/2014 11:05 PM, Aline Manera wrote:

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'):

-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center

Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 01/06/2014 11:05 PM, Aline Manera wrote:

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):

-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center

Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 01/06/2014 11:05 PM, Aline Manera wrote:

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 df9619f..bff0a18 100644 --- a/src/kimchi/exception.py +++ b/src/kimchi/exception.py @@ -39,3 +39,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 a21fcf7..f48f3d3 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.iscsi import TargetClient from kimchi.networkxml import to_network_xml @@ -1184,7 +1184,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 5d31f2a..1720384 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

-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center

Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Just a minor comment below On 01/06/2014 11:05 PM, Aline Manera wrote:

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 | 42 ++++++++++++++++-------------------------- src/kimchi/utils.py | 12 ++++++++++++ 2 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py index 7d919a0..f93ff0e 100644 --- a/src/kimchi/isoinfo.py +++ b/src/kimchi/isoinfo.py @@ -29,7 +29,7 @@ import urllib2

from kimchi.exception import IsoFormatError -from kimchi.utils import kimchi_log +from kimchi.utils import check_url_path, kimchi_log

iso_dir = [ ## @@ -134,13 +134,22 @@ 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): self.path = path + self.remote = self._is_iso_remote() self.volume_id = None self.bootable = False - self.remote = remote self._scan()

+ def _is_iso_remote(self): + if os.path.isfile(self.path): + return False + + if check_url_path(self.path): + return True + + raise IsoFormatError('ISO %s does not exist' % self.path) + def _unpack(self, s, data): return s.unpack(data[:s.size])

@@ -246,9 +255,9 @@ class Matcher(object): return self.lastmatch.group(num)

-def _probe_iso(fname, remote = False): +def _probe_iso(fname): try: - iso = IsoImage(fname, remote) + iso = IsoImage(fname) except Exception, e: kimchi_log.warning("probe_iso: Error processing ISO image: %s\n%s" % (fname, e)) @@ -302,35 +311,16 @@ def probe_iso(status_helper, params): 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__': diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index b6d84fd..af245c6 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -23,6 +23,7 @@

import cherrypy import os +import urllib2

from cherrypy.lib.reprconf import Parser @@ -84,3 +85,14 @@ def import_class(class_path):

def import_module(module_name): return __import__(module_name, globals(), locals(), ['']) + + +def check_url_path(path): + try: + code = urllib2.urlopen(path).getcode() + if code != 200: + return False + except (urllib2.URLError, ValueError): + return False + + return True

the above is right to me. not sure the follow code is the same as above. you can ignore the follow code. +def check_url_path(path): + try: + code = urllib2.urlopen(path).getcode() + return code != 200 + except (urllib2.URLError, ValueError): + return False -- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center

Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 01/06/2014 11:05 PM, Aline Manera wrote:

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 f93ff0e..a992a68 100644 --- a/src/kimchi/isoinfo.py +++ b/src/kimchi/isoinfo.py @@ -150,6 +150,26 @@ class IsoImage(object):

raise IsoFormatError('ISO %s does not exist' % self.path)

+ 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])

@@ -255,33 +275,6 @@ class Matcher(object): return self.lastmatch.group(num)

-def _probe_iso(fname): - try: - iso = IsoImage(fname) - 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'] @@ -307,22 +300,20 @@ def probe_iso(status_helper, params): continue iso = os.path.join(root, name) try: - ret = _probe_iso(iso) + 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 f48f3d3..7115583 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 @@ -66,6 +65,7 @@ from kimchi.exception import InvalidOperation, InvalidParameter, IsoFormatError from kimchi.exception import MissingParameter, NotFoundError, OperationFailed from kimchi.featuretests import FeatureTests from kimchi.iscsi import TargetClient +from kimchi.isoinfo import IsoImage from kimchi.networkxml import to_network_xml from kimchi.objectstore import ObjectStore from kimchi.scan import Scanner @@ -1182,7 +1182,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 1720384..69af6ec 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)

-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center

Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 01/06/2014 11:05 PM, Aline Manera wrote:

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 | 27 ++++++++++++++++----------- 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/Makefile.am b/Makefile.am index 7ab1bd8..04ad696 100644 --- a/Makefile.am +++ b/Makefile.am @@ -51,6 +51,7 @@ PEP8_WHITELIST = \ src/kimchi/exception.py \ src/kimchi/featuretests.py \ src/kimchi/iscsi.py \ + src/kimchi/isoinfo.py \ src/kimchi/kvmusertests.py \ src/kimchi/rollbackcontext.py \ src/kimchi/root.py \ diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py index a992a68..7e6d733 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 check_url_path, kimchi_log

+ iso_dir = [ ## # Portions of this data from libosinfo: http://libosinfo.org/ @@ -165,8 +166,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')

@@ -177,9 +178,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): @@ -199,7 +200,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 @@ -227,8 +229,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: @@ -238,7 +240,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: @@ -310,13 +313,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

-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center