[PATCH] fix for bug 12292

Alexander Bokovoy ab at samba.org
Wed Sep 28 13:00:41 UTC 2016


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.
-- 
/ Alexander Bokovoy



More information about the samba-technical mailing list