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

[Abhijeet Kasurde]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150819/dfecdae4/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akasurde-0001-4-try-except-block-in-ipautil.patch
Type: text/x-patch
Size: 3053 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150819/dfecdae4/attachment.bin>