This results in

  $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
  FROM debian:9
  ./lcitool: Unsupported architecture ppc64el

being printed on error, instead of the much nastier

  $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
  FROM debian:9
  Traceback (most recent call last):
    File "./lcitool", line 704, in <module>
      Application().run()
    File "./lcitool", line 699, in run
      args.func(args)
    File "./lcitool", line 643, in _action_dockerfile
      deb_arch = Util.native_arch_to_deb_arch(args.cross_arch)
    File "./lcitool", line 126, in native_arch_to_deb_arch
      raise Exception("Unsupported architecture {}".format(native_arch))
  Exception: Unsupported architecture foo

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 guests/lcitool | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index d6c8105..1119aaf 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -105,7 +105,7 @@ class Util:
             "x86_64": "x86_64-linux-gnu",
         }
         if native_arch not in archmap:
-            raise Exception("Unsupported architecture {}".format(native_arch))
+            raise Error("Unsupported architecture {}".format(native_arch))
         return archmap[native_arch]
 
     @staticmethod
@@ -123,7 +123,7 @@ class Util:
             "x86_64": "amd64",
         }
         if native_arch not in archmap:
-            raise Exception("Unsupported architecture {}".format(native_arch))
+            raise Error("Unsupported architecture {}".format(native_arch))
         return archmap[native_arch]
 
 
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Mar 11, 2019 at 06:48:11PM +0100, Andrea Bolognani wrote:
> This results in
> 
>   $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
>   FROM debian:9
>   ./lcitool: Unsupported architecture ppc64el
> 
> being printed on error, instead of the much nastier
> 
>   $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
>   FROM debian:9
>   Traceback (most recent call last):
>     File "./lcitool", line 704, in <module>
>       Application().run()
>     File "./lcitool", line 699, in run
>       args.func(args)
>     File "./lcitool", line 643, in _action_dockerfile
>       deb_arch = Util.native_arch_to_deb_arch(args.cross_arch)
>     File "./lcitool", line 126, in native_arch_to_deb_arch
>       raise Exception("Unsupported architecture {}".format(native_arch))
>   Exception: Unsupported architecture foo

I'm curious why the "Error" class exists at all ? It doesn't seem
to add anything that the normal "Exception" class can't do, and
leads to bugs like the one here.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, 2019-03-11 at 17:55 +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 11, 2019 at 06:48:11PM +0100, Andrea Bolognani wrote:
> > This results in
> > 
> >   $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
> >   FROM debian:9
> >   ./lcitool: Unsupported architecture ppc64el
> > 
> > being printed on error, instead of the much nastier
> > 
> >   $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
> >   FROM debian:9
> >   Traceback (most recent call last):
> >     File "./lcitool", line 704, in <module>
> >       Application().run()
> >     File "./lcitool", line 699, in run
> >       args.func(args)
> >     File "./lcitool", line 643, in _action_dockerfile
> >       deb_arch = Util.native_arch_to_deb_arch(args.cross_arch)
> >     File "./lcitool", line 126, in native_arch_to_deb_arch
> >       raise Exception("Unsupported architecture {}".format(native_arch))
> >   Exception: Unsupported architecture foo
> 
> I'm curious why the "Error" class exists at all ? It doesn't seem
> to add anything that the normal "Exception" class can't do, and
> leads to bugs like the one here.

I seem to understand you're not supposed to use Exception directly,
but rather define your own exception types:

  https://docs.python.org/3/tutorial/errors.html#user-defined-exceptions

I remember reading more about this, but I can't find the source
right now.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Mar 11, 2019 at 07:11:49PM +0100, Andrea Bolognani wrote:
> On Mon, 2019-03-11 at 17:55 +0000, Daniel P. Berrangé wrote:
> > On Mon, Mar 11, 2019 at 06:48:11PM +0100, Andrea Bolognani wrote:
> > > This results in
> > > 
> > >   $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
> > >   FROM debian:9
> > >   ./lcitool: Unsupported architecture ppc64el
> > > 
> > > being printed on error, instead of the much nastier
> > > 
> > >   $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
> > >   FROM debian:9
> > >   Traceback (most recent call last):
> > >     File "./lcitool", line 704, in <module>
> > >       Application().run()
> > >     File "./lcitool", line 699, in run
> > >       args.func(args)
> > >     File "./lcitool", line 643, in _action_dockerfile
> > >       deb_arch = Util.native_arch_to_deb_arch(args.cross_arch)
> > >     File "./lcitool", line 126, in native_arch_to_deb_arch
> > >       raise Exception("Unsupported architecture {}".format(native_arch))
> > >   Exception: Unsupported architecture foo
> > 
> > I'm curious why the "Error" class exists at all ? It doesn't seem
> > to add anything that the normal "Exception" class can't do, and
> > leads to bugs like the one here.
> 
> I seem to understand you're not supposed to use Exception directly,
> but rather define your own exception types:
> 
>   https://docs.python.org/3/tutorial/errors.html#user-defined-exceptions
> 
> I remember reading more about this, but I can't find the source
> right now.

I think it largely depends on the scope / scale of the application you
are writing. If the code is complex enough that you're going to want
to be catching exceptions in specific scenarios, then creating subclasses
makes sense as you can avoid mistakenly catching too much exceptions.

In the lcitool case though, we're not trying to do anything very clever
with catching specific exceptions as our app is quite small & self
contained. We just want to catch everything to hide the ugly stack traces
by default. IMHO in that scenario it is fine to just use the base exception
class directly. BTW, there's no need for the "message" field, even with
the Error class, as the default Exception class will stringify to report
just the message it received: eg this is equiv:

@@ -703,5 +700,5 @@ if __name__ == "__main__":
     try:
         Application().run()
     except Error as err:
-        sys.stderr.write("{}: {}\n".format(sys.argv[0], err.message))
+        sys.stderr.write("{}: {}\n".format(sys.argv[0], err))
         sys.exit(1)

anyway, I think we can just simplify our life & delete the Error class.

It would probably be worth registering a "--debug" flag to the CLI to
disable the global  exception catch, so that we can see the full stack
trace when needed.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, 2019-03-11 at 18:24 +0000, Daniel P. Berrangé wrote:
> On Mon, Mar 11, 2019 at 07:11:49PM +0100, Andrea Bolognani wrote:
> > On Mon, 2019-03-11 at 17:55 +0000, Daniel P. Berrangé wrote:
> > > I'm curious why the "Error" class exists at all ? It doesn't seem
> > > to add anything that the normal "Exception" class can't do, and
> > > leads to bugs like the one here.
> > 
> > I seem to understand you're not supposed to use Exception directly,
> > but rather define your own exception types:
> > 
> >   https://docs.python.org/3/tutorial/errors.html#user-defined-exceptions
> > 
> > I remember reading more about this, but I can't find the source
> > right now.
> 
> I think it largely depends on the scope / scale of the application you
> are writing. If the code is complex enough that you're going to want
> to be catching exceptions in specific scenarios, then creating subclasses
> makes sense as you can avoid mistakenly catching too much exceptions.
> 
> In the lcitool case though, we're not trying to do anything very clever
> with catching specific exceptions as our app is quite small & self
> contained. We just want to catch everything to hide the ugly stack traces
> by default. IMHO in that scenario it is fine to just use the base exception
> class directly. BTW, there's no need for the "message" field, even with
> the Error class, as the default Exception class will stringify to report
> just the message it received: eg this is equiv:
> 
> @@ -703,5 +700,5 @@ if __name__ == "__main__":
>      try:
>          Application().run()
>      except Error as err:
> -        sys.stderr.write("{}: {}\n".format(sys.argv[0], err.message))
> +        sys.stderr.write("{}: {}\n".format(sys.argv[0], err))
>          sys.exit(1)
> 
> anyway, I think we can just simplify our life & delete the Error class.
> 
> It would probably be worth registering a "--debug" flag to the CLI to
> disable the global  exception catch, so that we can see the full stack
> trace when needed.

I don't have a problem with any of the changes you describe. I'm
far from being an expert at Python, so having used suboptimal
constructs when implementing lcitool doesn't surprise me one bit :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Mar 11, 2019 at 07:11:49PM +0100, Andrea Bolognani wrote:
> On Mon, 2019-03-11 at 17:55 +0000, Daniel P. Berrangé wrote:
> > On Mon, Mar 11, 2019 at 06:48:11PM +0100, Andrea Bolognani wrote:
> > > This results in
> > >
> > >   $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
> > >   FROM debian:9
> > >   ./lcitool: Unsupported architecture ppc64el
> > >
> > > being printed on error, instead of the much nastier
> > >
> > >   $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
> > >   FROM debian:9
> > >   Traceback (most recent call last):
> > >     File "./lcitool", line 704, in <module>
> > >       Application().run()
> > >     File "./lcitool", line 699, in run
> > >       args.func(args)
> > >     File "./lcitool", line 643, in _action_dockerfile
> > >       deb_arch = Util.native_arch_to_deb_arch(args.cross_arch)
> > >     File "./lcitool", line 126, in native_arch_to_deb_arch
> > >       raise Exception("Unsupported architecture {}".format(native_arch))
> > >   Exception: Unsupported architecture foo
> >
> > I'm curious why the "Error" class exists at all ? It doesn't seem
> > to add anything that the normal "Exception" class can't do, and
> > leads to bugs like the one here.
>
> I seem to understand you're not supposed to use Exception directly,
> but rather define your own exception types:
>
>   https://docs.python.org/3/tutorial/errors.html#user-defined-exceptions
>
> I remember reading more about this, but I can't find the source
> right now.

Hmm, I've been told to subclass the Exception only if the module you're writing
is supposed to be a library/package someone else would like to depend on. But
in case of a standalone cli app (which this is), Exception should be all we
need.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list