Reviewed-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> On 01/04/2014 03:18 AM, 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

Just a minor comment below On 01/04/2014 03:18 AM, 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 | 44 +++++++++++++++++++------------------------- src/kimchi/utils.py | 12 ++++++++++++ 2 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py index 7d919a0..83e4b95 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,26 @@ 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 why change "remote = False " to "remote = None" Sometimes, you want to check the iso is remote by caller outer of the c And Sometimes, you want to check the iso is remote by IsoImage

+ 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 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,7 +259,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 +311,20 @@ def probe_iso(status_helper, params): continue iso = os.path.join(root, name) try: - ret = _probe_iso(iso) + ret = _probe_iso(iso, False)

why not we remove "remote" argument, and always class IsoImage(object): + def __init__(self, path, remote = False ): + self.remote = self._is_iso_remote() + + def _is_iso_remote(self): + if os.path.isfile(self.path): + return False ..... or just let the caller of IsoImage to check it? class IsoImage(object): + def __init__(self, path, remote = False ): then isoimg = IsoImage(path, remote = is_iso_remote(path) ): + def _is_iso_remote(path): + if os.path.isfile(path): + return False + + if check_url_path(path): + return True + + raise IsoFormatError('ISO %s does not exist' % path) the same question of IsoImage definition

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..efd693c 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.HTTPError, ValueError): + return False + + return True

+def check_url_path(path): + try: + code = urllib2.urlopen(path).getcode() + return code != 200 + except (urllib2.HTTPError, ValueError): + return False when urllib2.HTTPError, ValueError occur? not sure urllib2.HTTPError, ValueError can cover all exception so other exception still throw? -- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center

Just a minor comment below On 01/04/2014 03:18 AM, 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 83e4b95..46e0cf3 100644 --- a/src/kimchi/isoinfo.py +++ b/src/kimchi/isoinfo.py @@ -154,6 +154,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)) log prefers comma. seems you copy it from the below code. :-) + + return ('unknown', 'unknown') + def _unpack(self, s, data): return s.unpack(data[:s.size])

@@ -259,33 +279,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'] @@ -311,22 +304,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 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

Just a minor comment below On 01/04/2014 03:18 AM, 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 | 29 +++++++++++++++++------------ 2 files changed, 18 insertions(+), 12 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 46e0cf3..b13e4e4 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/ @@ -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 @@ -169,8 +170,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)) log prefers comma kimchi_log.debug(msg, self.path, self.volume_id)

return ('unknown', 'unknown')

@@ -181,9 +182,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): @@ -203,7 +204,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 @@ -231,8 +233,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: @@ -242,7 +244,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: @@ -314,13 +317,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