[PATCH] fix for bug 12292

[Alexander Bokovoy]

On ke, 28 syys 2016, Rowland Penny wrote:
> On Wed, 28 Sep 2016 16:00:41 +0300
> Alexander Bokovoy <ab at samba.org> wrote:
> 
> > On ke, 28 syys 2016, Rowland Penny wrote:
> > > On Wed, 28 Sep 2016 23:20:20 +1300
> > > Douglas Bagnall <douglas.bagnall at catalyst.net.nz> wrote:
> > > 
> > > > On 28/09/16 22:24, Rowland Penny wrote:
> > > > 
> > > > >> The attached patch is close to the mimimum necessary to fix the
> > > > >> bug as described on bugzilla. Further refactoring is not
> > > > >> necessarily bad, but it doesn't need to sneak in under bug
> > > > >> 12292's banner.
> > > > >>
> > > > >> cheers,
> > > > >> Douglas
> > > > > 
> > > > > Might be being a bit thick here, but couldn't you not bother
> > > > > with the new class and just use:
> > > > > 
> > > > > +        except:
> > > > > +            self.outf.write("Unable to find user %s\n" %
> > > > > username)
> > > > > 
> > > > 
> > > > The trouble there is you would catch all exceptions, regardless of
> > > > their origin, which would hide real errors. For example, the
> > > > search or delete could fail, or the "assert(len(target) == 1)"
> > > > could raise an AssertionError. You need to treat generic
> > > > exceptions as errors and filter out the special cases for special
> > > > treatment.
> > > > 
> > > > Exception subclasses are a good (simple, unambiguous, idiomatic)
> > > > way of communicating this kind of thing. We should make use of
> > > > them but we very rarely do.
> > > > 
> > > > Douglas
> > > 
> > > OK, I changed these lines:
> > > 
> > >         except:
> > >             raise CommandError('Unable to find user "%s"' %
> > > (username))
> > > 
> > > To these:
> > > 
> > >         except Exception, e:
> > >             raise CommandError('Unable to find user "%s"' %
> > > (username), e)
> > > 
> > > With the former, trying to delete a user that doesn't exist, you get
> > > this:
> > > 
> > > # samba-tool user delete unknown
> > > ERROR: Unable to find user "unknown"
> > > 
> > > With the later, you get this:
> > > 
> > > # samba-tool user delete unknown
> > > ERROR(<type 'exceptions.IndexError'>): Unable to find user
> > > "unknown" - list index out of range File
> > > "/usr/local/samba/lib/python2.7/site-packages/samba/netcmd/user.py",
> > > line 324, in run user_dn = res[0].dn
> > > 
> > > Does that really tell the user anything more ?
> > > Yes it tells the user just why technically it failed, the list index
> > > is out of range, but this is because the user doesn't exist!
> > Perhaps we could have some common handling in the CommandError class.
> > 
> > However, I think Douglas was trying to point out that it is bad
> > practice in Python programming to have 'except' without specific
> > exception -- catch-all cases are almost every time wrong. So if you
> > know that there will be an index error, just catch that exception
> > type to handle it properly.
> 
> Well yes, but I only found out what the exception was by adding
> 'Exception, e' to the line i.e. I hadn't a clue
> 
> I personally think you are all getting bogged down in technicalities
> here, what does it matter why the user couldn't be found, it couldn't
> be found!
Correct. However, if there would be another reason, like genuine error
in the underlying software stack, the 'except IndexError, e' exception
handling would allow us to crash with a proper backtrace to see that
issue. A plain 'except:' or 'except Exception, e' will cover up any
error and will complicate debugging. That's one point why we insist on
this technicality.

-- 
/ Alexander Bokovoy