[Freeipa-devel] [PATCH] Added try/except for error handling ipautil

[Martin Basti]

On 08/19/2015 01:49 PM, Abhijeet Kasurde wrote:
> Hi All,
>
>
> Please find the latest patch with review comments included.
>
> Thanks Martin for your help and review comments.
>
> Thanks,
> Abhijeet Kasurde
>
> On 08/19/2015 05:08 PM, Martin Basti wrote:
>>
>>
>> On 08/17/2015 02:08 PM, Abhijeet Kasurde wrote:
>>> Hi All,
>>>
>>> I have included all review comments. Please let me know your views.
>>>
>>> On 08/17/2015 04:09 PM, Martin Basti wrote:
>>>>
>>>>
>>>> On 08/17/2015 11:11 AM, Abhijeet Kasurde wrote:
>>>>> Hi All,
>>>>>
>>>>> Please find the update patch with review comments,
>>>>>
>>>>>
>>>>> On 08/14/2015 05:19 PM, Martin Basti wrote:
>>>>>>
>>>>>>
>>>>>> On 08/14/2015 06:57 AM, Abhijeet Kasurde wrote:
>>>>>>>
>>>>>>> On 08/13/2015 07:08 PM, Martin Basti wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 08/10/2015 01:47 PM, Abhijeet Kasurde wrote:
>>>>>>>>> Hi All,
>>>>>>>>>
>>>>>>>>> This patch fixes bug - 
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3406
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Abhijeet Kasurde
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> thank you for the patch
>>>>>>>>
>>>>>>>> 1)
>>>>>>>> -            except ValueError:
>>>>>>>> +            except EOFError, ValueError:
>>>>>>>>
>>>>>>>> Please use
>>>>>>>> except (EOFError, ValueError):
>>>>>>>> https://docs.python.org/2/tutorial/errors.html#handling-exceptions
>>>>>>> OK, I will include this.
>>>>>>>> 2)
>>>>>>>> I'm not sure if this code will work (I did not test it)
>>>>>>>>
>>>>>>>> I expect when stdin is closed, this will result into infinite 
>>>>>>>> loop, because raw_input will always return EOFError.
>>>>>>>>
>>>>>>>>         while True:
>>>>>>>>             try:
>>>>>>>>                 ret = raw_input("%s: " % prompt)
>>>>>>>>                 if allow_empty or ret.strip():
>>>>>>>>                     return ret
>>>>>>>>             except EOFError:
>>>>>>>>                 pass
>>>>>>>>
>>>>>>> Could you please elaborate more on, so that I can include fix in 
>>>>>>> this section of code?
>>>>>> If you receive EOF you cannot continue in while cycle because, it 
>>>>>> will return EOF every iteration forever.
>>>>>>
>>>>>> If EOF is received the while cycle must end, and appropriate 
>>>>>> action must be take.
>>>>>> It depends on situation, if default value is present then default 
>>>>>> value should be used, or in case if empty value is allowed, empty 
>>>>>> string should be returned.
>>>>>>
>>>>>> In case there is no default value and empty value is not allowed, 
>>>>>> then an exception should be raised.
>>>>>>
>>>>>> Martin^2
>>>>>
>>>> NACK
>>>>
>>>> There is still infinity loop.
>>>> 1)
>>>> +            except EOFError:
>>>> +                if allow_empty:
>>>> +                    return ''
>>>>
>>>> This will continue in while cycle if allow_empty=False, you need to raise exception there.
>>>>
>>>> 2)
>>>> Why so complicated? just return default looks enough to me.
>>>> +            except EOFError:
>>>> +                if choice.lower()[0] == "y":
>>>> +                    return True
>>>> +                elif choice.lower()[0] == "n":
>>>> +                    return False
>>>> 3)
>>>> Remove this change please
>>>> -
>>>>   def get_gsserror(e):
>>>>
>>>
>>>
>>>
>> NACK
>>
>> 1)
>> Your commit message does not reflect the changes you made in code. 
>> The patch just solves the EOFError not Value error or "various errors"
>>
>> 2)
>> You removed if statement which should be there. I expected something 
>> like this in first case.
>>           while True:
>> -            ret = raw_input("%s: " % prompt)
>> -            if allow_empty or ret.strip():
>> -                return ret
>> +            try:
>> +                ret = raw_input("%s: " % prompt)
>> +                if allow_empty or ret.strip():
>> +                    return ret
>> +            except EOFError:
>> +                if allow_empty:
>> +                   return ''
>> +                raise RuntimeError("Failed to get user input")
>> Thanks
>> Martin
>

ACK

Pushed to master: 7c48621bb8e9efc47c68bb7b4af936da93325050
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150819/d3c98dd9/attachment.htm>