<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> Hi All, Please find the latest patch with review comments included. Thanks Martin for your help and review comments. Thanks, Abhijeet Kasurde <div class="moz-cite-prefix">On 08/19/2015 05:08 PM, Martin Basti wrote: </div> <blockquote cite="mid:55D46AC6.9080803@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 08/17/2015 02:08 PM, Abhijeet Kasurde wrote: </div> <blockquote cite="mid:55D1CEBF.3010405@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> Hi All, I have included all review comments. Please let me know your views. <div class="moz-cite-prefix">On 08/17/2015 04:09 PM, Martin Basti wrote: </div> <blockquote cite="mid:55D1B9DF.4060109@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 08/17/2015 11:11 AM, Abhijeet Kasurde wrote: </div> <blockquote cite="mid:55D1A531.4020309@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> Hi All, Please find the update patch with review comments, <div class="moz-cite-prefix">On 08/14/2015 05:19 PM, Martin Basti wrote: </div> <blockquote cite="mid:55CDD5B9.1050105@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 08/14/2015 06:57 AM, Abhijeet Kasurde wrote: </div> <blockquote cite="mid:55CD7524.9080307@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 08/13/2015 07:08 PM, Martin Basti wrote: </div> <blockquote cite="mid:55CC9DB9.5000705@redhat.com" type="cite"> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> <div class="moz-cite-prefix">On 08/10/2015 01:47 PM, Abhijeet Kasurde wrote: </div> <blockquote cite="mid:55C88F57.6000500@redhat.com" type="cite">Hi All, This patch fixes bug - <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/3406">https://fedorahosted.org/freeipa/ticket/3406</a> Thanks, Abhijeet Kasurde <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> Hello, thank you for the patch 1) - except ValueError: + except EOFError, ValueError: Please use except (EOFError, ValueError): <a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://docs.python.org/2/tutorial/errors.html#handling-exceptions">https://docs.python.org/2/tutorial/errors.html#handling-exceptions</a> </blockquote> OK, I will include this. <blockquote cite="mid:55CC9DB9.5000705@redhat.com" type="cite"> 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 </blockquote> Could you please elaborate more on, so that I can include fix in this section of code? </blockquote> 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 </blockquote> </blockquote> NACK There is still infinity loop. 1) <pre wrap="">+ 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 </pre> 3) Remove this change please <pre wrap="">- def get_gsserror(e):</pre> </blockquote> <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> 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. <pre wrap=""> 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")</pre> Thanks Martin </blockquote> </body> </html>