| Message ID | 20181004194108.29485-1-yuli@crunchydata.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | libsemanage: improve semanage_migrate_store import failure | expand |
On Thu, Oct 4, 2018 at 12:46 PM Yuli Khodorkovskiy < yuli.khodorkovskiy@crunchydata.com> wrote: > The python module import error in semanage_migrate_store was misleading. > Before, it would print that the module is not installed, even though > it is in fact on the system. > > Now the python module import failure is correctly reported if the module > is not installed or the exact reason for failure is reported to the user. > > Signed-off-by: Yuli Khodorkovskiy <yuli@crunchydata.com> > --- > libsemanage/utils/semanage_migrate_store | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/libsemanage/utils/semanage_migrate_store > b/libsemanage/utils/semanage_migrate_store > index 2e6cb278..50eb59ef 100755 > --- a/libsemanage/utils/semanage_migrate_store > +++ b/libsemanage/utils/semanage_migrate_store > @@ -15,10 +15,12 @@ sepol = ctypes.cdll.LoadLibrary('libsepol.so.1') > try: > import selinux > import semanage > -except: > +except ImportError: > print("You must install libselinux-python and libsemanage-python > before running this tool", file=sys.stderr) > exit(1) > - > +except Exception as e: > + print("Failed to import libselinux-python/libsemanage-python: %s" > % str(e)) > + exit(1) > We should really only be handling exceptions we reasonably expect and discourage the usage of catching raw Exception, especially considering not-catching this will cause the runtime to print a stack trace, the error and exit non-zero. We probably only need the except ImportError change and can drop the second hunk. Does anyone disagree with this? > > def copy_file(src, dst): > if DEBUG: > -- > 2.19.0 > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to > Selinux-request@tycho.nsa.gov. > <div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Oct 4, 2018 at 12:46 PM Yuli Khodorkovskiy <<a href="mailto:yuli.khodorkovskiy@crunchydata.com">yuli.khodorkovskiy@crunchydata.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The python module import error in semanage_migrate_store was misleading.<br> Before, it would print that the module is not installed, even though<br> it is in fact on the system.<br> <br> Now the python module import failure is correctly reported if the module<br> is not installed or the exact reason for failure is reported to the user.<br> <br> Signed-off-by: Yuli Khodorkovskiy <<a href="mailto:yuli@crunchydata.com" target="_blank">yuli@crunchydata.com</a>><br> ---<br> libsemanage/utils/semanage_migrate_store | 6 ++++--<br> 1 file changed, 4 insertions(+), 2 deletions(-)<br> <br> diff --git a/libsemanage/utils/semanage_migrate_store b/libsemanage/utils/semanage_migrate_store<br> index 2e6cb278..50eb59ef 100755<br> --- a/libsemanage/utils/semanage_migrate_store<br> +++ b/libsemanage/utils/semanage_migrate_store<br> @@ -15,10 +15,12 @@ sepol = ctypes.cdll.LoadLibrary('libsepol.so.1')<br> try:<br> import selinux<br> import semanage<br> -except:<br> +except ImportError:<br> print("You must install libselinux-python and libsemanage-python before running this tool", file=sys.stderr)<br> exit(1)<br> -<br> +except Exception as e:<br> + print("Failed to import libselinux-python/libsemanage-python: %s" % str(e))<br> + exit(1)<br></blockquote><div><br></div><div>We should really only be handling exceptions we reasonably expect and discourage</div><div>the usage of catching raw Exception, especially considering not-catching this will</div><div>cause the runtime to print a stack trace, the error and exit non-zero.</div><div><br></div><div>We probably only need the except ImportError change and can drop the second hunk.</div><div><br></div><div>Does anyone disagree with this?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> def copy_file(src, dst):<br> if DEBUG:<br> -- <br> 2.19.0<br> <br> _______________________________________________<br> Selinux mailing list<br> <a href="mailto:Selinux@tycho.nsa.gov" target="_blank">Selinux@tycho.nsa.gov</a><br> To unsubscribe, send email to <a href="mailto:Selinux-leave@tycho.nsa.gov" target="_blank">Selinux-leave@tycho.nsa.gov</a>.<br> To get help, send an email containing "help" to <a href="mailto:Selinux-request@tycho.nsa.gov" target="_blank">Selinux-request@tycho.nsa.gov</a>.<br> </blockquote></div></div>
On Fri, Oct 05, 2018 at 07:13:23AM -0700, William Roberts wrote: > On Thu, Oct 4, 2018 at 12:46 PM Yuli Khodorkovskiy < > yuli.khodorkovskiy@crunchydata.com> wrote: > > > The python module import error in semanage_migrate_store was misleading. > > Before, it would print that the module is not installed, even though > > it is in fact on the system. > > > > Now the python module import failure is correctly reported if the module > > is not installed or the exact reason for failure is reported to the user. > > > > Signed-off-by: Yuli Khodorkovskiy <yuli@crunchydata.com> > > --- > > libsemanage/utils/semanage_migrate_store | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/libsemanage/utils/semanage_migrate_store > > b/libsemanage/utils/semanage_migrate_store > > index 2e6cb278..50eb59ef 100755 > > --- a/libsemanage/utils/semanage_migrate_store > > +++ b/libsemanage/utils/semanage_migrate_store > > @@ -15,10 +15,12 @@ sepol = ctypes.cdll.LoadLibrary('libsepol.so.1') > > try: > > import selinux > > import semanage > > -except: > > +except ImportError: > > print("You must install libselinux-python and libsemanage-python > > before running this tool", file=sys.stderr) > > exit(1) > > - > > +except Exception as e: > > + print("Failed to import libselinux-python/libsemanage-python: %s" > > % str(e)) > > + exit(1) > > > > We should really only be handling exceptions we reasonably expect and > discourage > the usage of catching raw Exception, especially considering not-catching > this will > cause the runtime to print a stack trace, the error and exit non-zero. > > We probably only need the except ImportError change and can drop the second > hunk. > > Does anyone disagree with this? Agreed. catching Exception is bad cuz it also catches KeyboardInterrupt and stuff like that. -- Jason > > > > > > def copy_file(src, dst): > > if DEBUG: > > -- > > 2.19.0 > > > > _______________________________________________ > > Selinux mailing list > > Selinux@tycho.nsa.gov > > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > > To get help, send an email containing "help" to > > Selinux-request@tycho.nsa.gov. > > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
On 10/05/2018 10:32 AM, Jason Zaman wrote: > On Fri, Oct 05, 2018 at 07:13:23AM -0700, William Roberts wrote: >> On Thu, Oct 4, 2018 at 12:46 PM Yuli Khodorkovskiy < >> yuli.khodorkovskiy@crunchydata.com> wrote: >> >>> The python module import error in semanage_migrate_store was misleading. >>> Before, it would print that the module is not installed, even though >>> it is in fact on the system. >>> >>> Now the python module import failure is correctly reported if the module >>> is not installed or the exact reason for failure is reported to the user. >>> >>> Signed-off-by: Yuli Khodorkovskiy <yuli@crunchydata.com> >>> --- >>> libsemanage/utils/semanage_migrate_store | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/libsemanage/utils/semanage_migrate_store >>> b/libsemanage/utils/semanage_migrate_store >>> index 2e6cb278..50eb59ef 100755 >>> --- a/libsemanage/utils/semanage_migrate_store >>> +++ b/libsemanage/utils/semanage_migrate_store >>> @@ -15,10 +15,12 @@ sepol = ctypes.cdll.LoadLibrary('libsepol.so.1') >>> try: >>> import selinux >>> import semanage >>> -except: >>> +except ImportError: >>> print("You must install libselinux-python and libsemanage-python >>> before running this tool", file=sys.stderr) >>> exit(1) >>> - >>> +except Exception as e: >>> + print("Failed to import libselinux-python/libsemanage-python: %s" >>> % str(e)) >>> + exit(1) >>> >> >> We should really only be handling exceptions we reasonably expect and >> discourage >> the usage of catching raw Exception, especially considering not-catching >> this will >> cause the runtime to print a stack trace, the error and exit non-zero. >> >> We probably only need the except ImportError change and can drop the second >> hunk. >> >> Does anyone disagree with this? For this case, I agree that ImportError is the only thing that should be caught. > Agreed. catching Exception is bad cuz it also catches KeyboardInterrupt > and stuff like that. That's not correct. Catching BaseException would catch KeyboardInterrupt. Catching Exception would not. See the Python builtin exception hierarchy: https://docs.python.org/3/library/exceptions.html#exception-hierarchy IMO catching Exception has valid uses.
Yuli, If you respin this with just import error looks like its a go. Bill On Fri, Oct 5, 2018 at 12:53 PM Chris PeBenito <pebenito@ieee.org> wrote: > On 10/05/2018 10:32 AM, Jason Zaman wrote: > > On Fri, Oct 05, 2018 at 07:13:23AM -0700, William Roberts wrote: > >> On Thu, Oct 4, 2018 at 12:46 PM Yuli Khodorkovskiy < > >> yuli.khodorkovskiy@crunchydata.com> wrote: > >> > >>> The python module import error in semanage_migrate_store was > misleading. > >>> Before, it would print that the module is not installed, even though > >>> it is in fact on the system. > >>> > >>> Now the python module import failure is correctly reported if the > module > >>> is not installed or the exact reason for failure is reported to the > user. > >>> > >>> Signed-off-by: Yuli Khodorkovskiy <yuli@crunchydata.com> > >>> --- > >>> libsemanage/utils/semanage_migrate_store | 6 ++++-- > >>> 1 file changed, 4 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/libsemanage/utils/semanage_migrate_store > >>> b/libsemanage/utils/semanage_migrate_store > >>> index 2e6cb278..50eb59ef 100755 > >>> --- a/libsemanage/utils/semanage_migrate_store > >>> +++ b/libsemanage/utils/semanage_migrate_store > >>> @@ -15,10 +15,12 @@ sepol = ctypes.cdll.LoadLibrary('libsepol.so.1') > >>> try: > >>> import selinux > >>> import semanage > >>> -except: > >>> +except ImportError: > >>> print("You must install libselinux-python and > libsemanage-python > >>> before running this tool", file=sys.stderr) > >>> exit(1) > >>> - > >>> +except Exception as e: > >>> + print("Failed to import libselinux-python/libsemanage-python: > %s" > >>> % str(e)) > >>> + exit(1) > >>> > >> > >> We should really only be handling exceptions we reasonably expect and > >> discourage > >> the usage of catching raw Exception, especially considering not-catching > >> this will > >> cause the runtime to print a stack trace, the error and exit non-zero. > >> > >> We probably only need the except ImportError change and can drop the > second > >> hunk. > >> > >> Does anyone disagree with this? > > For this case, I agree that ImportError is the only thing that should be > caught. > > > Agreed. catching Exception is bad cuz it also catches KeyboardInterrupt > > and stuff like that. > > That's not correct. Catching BaseException would catch > KeyboardInterrupt. Catching Exception would not. See the Python > builtin exception hierarchy: > > https://docs.python.org/3/library/exceptions.html#exception-hierarchy > > IMO catching Exception has valid uses. > > -- > Chris PeBenito > <div dir="ltr">Yuli,<div>If you respin this with just import error looks like its a go.</div><div>Bill</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Oct 5, 2018 at 12:53 PM Chris PeBenito <<a href="mailto:pebenito@ieee.org">pebenito@ieee.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 10/05/2018 10:32 AM, Jason Zaman wrote:<br> > On Fri, Oct 05, 2018 at 07:13:23AM -0700, William Roberts wrote:<br> >> On Thu, Oct 4, 2018 at 12:46 PM Yuli Khodorkovskiy <<br> >> <a href="mailto:yuli.khodorkovskiy@crunchydata.com" target="_blank">yuli.khodorkovskiy@crunchydata.com</a>> wrote:<br> >><br> >>> The python module import error in semanage_migrate_store was misleading.<br> >>> Before, it would print that the module is not installed, even though<br> >>> it is in fact on the system.<br> >>><br> >>> Now the python module import failure is correctly reported if the module<br> >>> is not installed or the exact reason for failure is reported to the user.<br> >>><br> >>> Signed-off-by: Yuli Khodorkovskiy <<a href="mailto:yuli@crunchydata.com" target="_blank">yuli@crunchydata.com</a>><br> >>> ---<br> >>> libsemanage/utils/semanage_migrate_store | 6 ++++--<br> >>> 1 file changed, 4 insertions(+), 2 deletions(-)<br> >>><br> >>> diff --git a/libsemanage/utils/semanage_migrate_store<br> >>> b/libsemanage/utils/semanage_migrate_store<br> >>> index 2e6cb278..50eb59ef 100755<br> >>> --- a/libsemanage/utils/semanage_migrate_store<br> >>> +++ b/libsemanage/utils/semanage_migrate_store<br> >>> @@ -15,10 +15,12 @@ sepol = ctypes.cdll.LoadLibrary('libsepol.so.1')<br> >>> try:<br> >>> import selinux<br> >>> import semanage<br> >>> -except:<br> >>> +except ImportError:<br> >>> print("You must install libselinux-python and libsemanage-python<br> >>> before running this tool", file=sys.stderr)<br> >>> exit(1)<br> >>> -<br> >>> +except Exception as e:<br> >>> + print("Failed to import libselinux-python/libsemanage-python: %s"<br> >>> % str(e))<br> >>> + exit(1)<br> >>><br> >><br> >> We should really only be handling exceptions we reasonably expect and<br> >> discourage<br> >> the usage of catching raw Exception, especially considering not-catching<br> >> this will<br> >> cause the runtime to print a stack trace, the error and exit non-zero.<br> >><br> >> We probably only need the except ImportError change and can drop the second<br> >> hunk.<br> >><br> >> Does anyone disagree with this?<br> <br> For this case, I agree that ImportError is the only thing that should be <br> caught.<br> <br> > Agreed. catching Exception is bad cuz it also catches KeyboardInterrupt<br> > and stuff like that.<br> <br> That's not correct. Catching BaseException would catch <br> KeyboardInterrupt. Catching Exception would not. See the Python <br> builtin exception hierarchy:<br> <br> <a href="https://docs.python.org/3/library/exceptions.html#exception-hierarchy" rel="noreferrer" target="_blank">https://docs.python.org/3/library/exceptions.html#exception-hierarchy</a><br> <br> IMO catching Exception has valid uses.<br> <br> -- <br> Chris PeBenito<br> </blockquote></div>
Weird Gmail removed my text box for plain text mode in Gmail, re-sending since it got filtered out of the mailing list. On Mon, Oct 8, 2018 at 9:09 AM William Roberts <bill.c.roberts@gmail.com> wrote: > > Yuli, > If you respin this with just import error looks like its a go. > Bill > > On Fri, Oct 5, 2018 at 12:53 PM Chris PeBenito <pebenito@ieee.org> wrote: >> >> On 10/05/2018 10:32 AM, Jason Zaman wrote: >> > On Fri, Oct 05, 2018 at 07:13:23AM -0700, William Roberts wrote: >> >> On Thu, Oct 4, 2018 at 12:46 PM Yuli Khodorkovskiy < >> >> yuli.khodorkovskiy@crunchydata.com> wrote: >> >> >> >>> The python module import error in semanage_migrate_store was misleading. >> >>> Before, it would print that the module is not installed, even though >> >>> it is in fact on the system. >> >>> >> >>> Now the python module import failure is correctly reported if the module >> >>> is not installed or the exact reason for failure is reported to the user. >> >>> >> >>> Signed-off-by: Yuli Khodorkovskiy <yuli@crunchydata.com> >> >>> --- >> >>> libsemanage/utils/semanage_migrate_store | 6 ++++-- >> >>> 1 file changed, 4 insertions(+), 2 deletions(-) >> >>> >> >>> diff --git a/libsemanage/utils/semanage_migrate_store >> >>> b/libsemanage/utils/semanage_migrate_store >> >>> index 2e6cb278..50eb59ef 100755 >> >>> --- a/libsemanage/utils/semanage_migrate_store >> >>> +++ b/libsemanage/utils/semanage_migrate_store >> >>> @@ -15,10 +15,12 @@ sepol = ctypes.cdll.LoadLibrary('libsepol.so.1') >> >>> try: >> >>> import selinux >> >>> import semanage >> >>> -except: >> >>> +except ImportError: >> >>> print("You must install libselinux-python and libsemanage-python >> >>> before running this tool", file=sys.stderr) >> >>> exit(1) >> >>> - >> >>> +except Exception as e: >> >>> + print("Failed to import libselinux-python/libsemanage-python: %s" >> >>> % str(e)) >> >>> + exit(1) >> >>> >> >> >> >> We should really only be handling exceptions we reasonably expect and >> >> discourage >> >> the usage of catching raw Exception, especially considering not-catching >> >> this will >> >> cause the runtime to print a stack trace, the error and exit non-zero. >> >> >> >> We probably only need the except ImportError change and can drop the second >> >> hunk. >> >> >> >> Does anyone disagree with this? >> >> For this case, I agree that ImportError is the only thing that should be >> caught. >> >> > Agreed. catching Exception is bad cuz it also catches KeyboardInterrupt >> > and stuff like that. >> >> That's not correct. Catching BaseException would catch >> KeyboardInterrupt. Catching Exception would not. See the Python >> builtin exception hierarchy: >> >> https://docs.python.org/3/library/exceptions.html#exception-hierarchy >> >> IMO catching Exception has valid uses. >> >> -- >> Chris PeBenito
diff --git a/libsemanage/utils/semanage_migrate_store b/libsemanage/utils/semanage_migrate_store index 2e6cb278..50eb59ef 100755 --- a/libsemanage/utils/semanage_migrate_store +++ b/libsemanage/utils/semanage_migrate_store @@ -15,10 +15,12 @@ sepol = ctypes.cdll.LoadLibrary('libsepol.so.1') try: import selinux import semanage -except: +except ImportError: print("You must install libselinux-python and libsemanage-python before running this tool", file=sys.stderr) exit(1) - +except Exception as e: + print("Failed to import libselinux-python/libsemanage-python: %s" % str(e)) + exit(1) def copy_file(src, dst): if DEBUG:
The python module import error in semanage_migrate_store was misleading. Before, it would print that the module is not installed, even though it is in fact on the system. Now the python module import failure is correctly reported if the module is not installed or the exact reason for failure is reported to the user. Signed-off-by: Yuli Khodorkovskiy <yuli@crunchydata.com> --- libsemanage/utils/semanage_migrate_store | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)