[PATCH] fix for bug 12292

[Rowland Penny]

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!

Rowland