Exception handling advise needed for samba-tool

[Jelmer Vernooij]

On Wed, Apr 10, 2013 at 02:10:06AM +1000, Andrew Bartlett wrote:
> On Tue, 2013-04-09 at 13:40 +0200, Jelmer Vernooij wrote:
> > On Tue, Apr 09, 2013 at 12:01:21AM -0700, Matthieu Patou wrote:
> > > On 04/08/2013 04:18 PM, Andrew Bartlett wrote:
> > > >I'm trying to make the exceptions from samba-tool domain classicupgrade
> > > >less intimidating, so I tried this:
> > > >
> > > >diff --git a/python/samba/netcmd/domain.py
> > > >b/python/samba/netcmd/domain.py
> > > >index 4ba305c..c47b7be 100644
> > > >--- a/python/samba/netcmd/domain.py
> > > >+++ b/python/samba/netcmd/domain.py
> > > >@@ -1313,9 +1313,12 @@ class cmd_domain_classicupgrade(Command):
> > > >          s3conf.load(smbconf)
> > > >          samba3 = Samba3(smbconf, s3conf)
> > > >-        logger.info("Provisioning")
> > > >-        upgrade_from_samba3(samba3, logger, targetdir,
> > > >session_info=system_session(),
> > > >-                            useeadb=eadb, dns_backend=dns_backend,
> > > >use_ntvfs=use_ntvfs)
> > > >+        logger.info("Preparing to Upgrade")
> > > >+        try:
> > > >+            upgrade_from_samba3(samba3, logger, targetdir,
> > > >session_info=system_session(),
> > > >+                                useeadb=eadb, dns_backend=dns_backend,
> > > >use_ntvfs=use_ntvfs)
> > > >+        except ProvisioningError, e:
> > > >+            raise CommandError("Upgrade failed: ", e)
> > > >But I still get:
> > > >
> > > >ERROR(<class 'samba.provision.ProvisioningError'>): Upgrade failed:  -
> > > >ProvisioningError: Could not open '/tmp/secrets.tdb', the Samba3 secrets
> > > >database: [Errno 2] No such file or directory.  Perhaps you specified
> > > >the incorrect smb.conf, --testparm or --dbdir option?
> > > >   File "bin/python/samba/netcmd/domain.py", line 1319, in run
> > > >     useeadb=eadb, dns_backend=dns_backend, use_ntvfs=use_ntvfs)
> > > >   File "bin/python/samba/upgrade.py", line 575, in upgrade_from_samba3
> > > >     raise ProvisioningError("Could not open '%s', the Samba3 secrets
> > > >database: %s.  Perhaps you specified the incorrect smb.conf, --testparm
> > > >or --dbdir option?" % (samba3.privatedir_path("secrets.tdb"), str(e)))
> > > >
> > > >Can you remind me of the correct syntax to turn that into just:
> > > >
> > > >Upgrade failed: Could not open '/tmp/secrets.tdb', the Samba3 secrets
> > > >database: [Errno 2] No such file or directory.  Perhaps you specified
> > > >the incorrect smb.conf, --testparm or --dbdir option?
> > > I think that as long as you raise an exception you will get the call
> > > trace, try to see if you can just print the exception or just get
> > > the error message.
> > > 
> > > You can have a look at samba_upgradeprovision around line 1160 I did
> > > that there.
> > The infrastructure is meant to deal with exceptions and display them properly.
> > They shouldn't be caught and written to standard output manually.
> > 
> > Having a single place where exceptions are handled means we can avoid
> > reinventing the wheel everywhere and e.g. have environment variables to
> > force the dumping of tracebacks or having Python bring up a debugger whenever
> > an exception is raised. It also makes whitebox testing of the code easier
> > since you don't have to parse the output but you can just catch the
> > exception and introspect it.
> > 
> > How samba-tool handles various exceptions should really be documented better.
> > Sorry. The ubershort summary:
> > 
> >  * CommandError exceptions are expected, should only be raised by the
> >    command code (not library code), should have a user-comprehensible
> >    description. They're meant for general user-visible errors. When they're
> >    printed there is no traceback printed.
> >  * Any other internal errors should be caught by the command code
> >    and re-raise a CommandError. You can attach an internal exception to a
> >    CommandError for debugging purposes.
> >  * Any internal errors that are not explicitly caught are reported with a
> >    traceback. They're the Python equivalent of a segmentation error.
> > 
> > (it's gotten more complicated over time, and we special case more exceptions
> >  than just CommandError now. I don't have the source code with me to
> >  look at the moment)
> 
> So, how do I get just the error string out of the ProvisioningError into
> the CommandError?

An Exception is an object like any other, and the message should be one of the
attributes. You can do something like:

try:
   samba_team.provision(food)
except ProvisionError, e:
   raise CommandError(e.message)

That said, we should be careful with what kinds of exceptions we convert this
way and whether we actually want to have the error message bubble up to the
user as-is. For ProvisioningError this behaviour probably makes sense, but
you wouldn't want to do this for KeyError.

Cheers,

Jelmer