[Kimchi-devel] [PATCH 4/6] Move ISO path validation to IsoImage()

[Sheldon]

Just a minor comment below

On 01/04/2014 03:18 AM, Aline Manera wrote:
> From: Aline Manera <alinefm at 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 at 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


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)
> +        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)
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 at linux.vnet.ibm.com>
IBM Linux Technology Center