qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Auditing QEMU to replace NULL with &error_abort


From: Daniel P . Berrangé
Subject: Re: Auditing QEMU to replace NULL with &error_abort
Date: Wed, 23 Jun 2021 18:24:23 +0100
User-agent: Mutt/2.0.7 (2021-05-04)

On Wed, Jun 23, 2021 at 04:39:18PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jun 23, 2021 at 07:31:13PM +0400, Marc-André Lureau wrote:
> > Hi
> > 
> > On Wed, Jun 23, 2021 at 7:10 PM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> > 
> > > On Wed, Jun 23, 2021 at 02:16:55PM +0200, Markus Armbruster wrote:
> > > > &error_abort has been a clear win for us.  &error_fatal too, when used
> > > > judiciously.  Marc-André tried to get both into GLib, unsuccessfully[2].
> > >
> > > ...snip...
> > >
> > > > [2] https://gitlab.gnome.org/GNOME/glib/-/issues/2288
> > >
> > > This doesn't actually suggest adding error_abort/fatal to GLib. Rather
> > > it adds a general callback hook to GLib. Biggest complaints there
> > > are around the callback concept and difficulty of safely using it,
> > > which I can't disagree with.
> > >
> > > I wonder if we would have more luck if we explicitly proposed the
> > > error_abort/fatal concept to GLib instead. At least that would not
> > > hit any of the complaints raised about the callback.
> > >
> > >
> > Without callbacks, it will be difficult to report errors back to the
> > monitor, or prettify it the way we want (since we would be using extended
> > GErrors for hints etc)
> > 
> > But we could have a more specific callback for that perhaps?
> > 
> > You are welcome to propose something else :)
> 
> I was thinking g_set_error would use g_warning/g_error to report
> it, and thus involve the g_log_default_handler callback, which
> we could provide to feed back into the monitor.

I did such a change:

  
https://gitlab.gnome.org/dberrange/glib/-/commit/b3e951183f319ef634845485f84deefcc36b4dc3

but when I come to document it, I notice the GLib is very strongly
of the opinion that GError *never* be used to report programmer
errors. 'error_abort' is precisely for programmer errors, so I
think it will be a challenge to get that accepted in GLib.
GLib wants you to use g_error or g_return_if_fail or g_assert
etc, and bypass GError entirely.

Adding 'error_fatal' is a more reasonable challenge, but if we
only get one of the two added to GLib that doesn't especially
help us.

I can see that error_abort is simpler than 4 line code pattern
that would be needed to declare Error & call abort, but I wonder
if we have solved the wrong problem. Should we have just not
used Error object at all for programmer errors like GLib suggests.

Picking one common example. Almost every caller of qemu_opts_create()
passes error_abort, and those which don't look like they probably
should. So what value is the Error object adding instead of just
having the method directly call g_error to report the programmer
error ?  The main one I see is that it redirects output to HMP,
but I think that would be achievable by a custom log handler in
glib. A few other very common repeated usages of error_abort
look similarly redundant to me.

Anyway, this all looks like a bigger can of worms than I expected.

What I think it does show though, is that NULL vs &error_ignore
is a fairly minor detail in the bigger scheme of things, and thus
I'm not convinced it is the best thing to spend time on if we want
to work on error reporting.

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]