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

[Royce Lv]

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.
>
>>> +
>>>       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
>