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

[Aline Manera]

On 01/03/2014 12:19 AM, Royce Lv wrote:
> On 2014年01月03日 03:44, Aline Manera wrote:
>> On 12/31/2013 04:44 AM, Royce Lv wrote:
>>> On 2013年12月31日 01:09, 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 |   55 
>>>> +++++++++++++++++++++++++++----------------------
>>>>   1 file changed, 30 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/src/kimchi/isoinfo.py b/src/kimchi/isoinfo.py
>>>> index 7b7fa78..dd26204 100644
>>>> --- a/src/kimchi/isoinfo.py
>>>> +++ b/src/kimchi/isoinfo.py
>>>> @@ -134,13 +134,37 @@ 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 remote is not None:
>>>> +            self.remote = remote
>>>> +        else:
>>>> +            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
>>> I kind of want this func moved outside for reusage of other module.
>>
>> It isn't used in other place because that I moved it to to IsoImage()
> Actually I'm asking it because part I'm doing (change CDROM), I will 
> depend on this function to validate path almost same as remote/local 
> iso except my path doesn't have to be an ISO, so put in utils.py can 
> avoid duplicate code.

Got it. I will do it in V3.

>>
>>>> +
>>>>       def _unpack(self, s, data):
>>>>           return s.unpack(data[:s.size])
>>>>
>>>> @@ -246,7 +270,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 +322,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)
>>> I think if we want to move remote url probe inside IsoImage class, 
>>> is there any need to pass 'remote' param outside? We can just dump it.
>>
>> In this case, I passed 'remote' parameter as we have already checked 
>> if it is local
>>
>> if os.path.isdir(loc):
>>    ....
>>
>>
>>>>                   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)
>>> At first we have probe_one and probe_iso originally to serve use 
>>> cases of just probe one iso and probe all isos under a directory, 
>>> now that you are refactor this part, I suggest that we dump this 
>>> probe_one function, and all use probe_iso.
>>>>
>>>>
>>>>   if __name__ == '__main__':
>>>
>>> _______________________________________________
>>> Kimchi-devel mailing list
>>> Kimchi-devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>
>