[PATCH] fix for bug 12292

[Rowland Penny]

On Wed, 28 Sep 2016 16:44:22 +0300
Alexander Bokovoy <ab at samba.org> wrote:

> 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.
> 

OK, that's me over there waving the white flag ;-)

I will alter the patch to use 'except IndexError, e' ,
but before I do, is there anything else wrong with my patch ???

Rowland