Re: [Qemu-devel] chardev's and fd's in monitors

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> "Daniel P. Berrange" <address@hidden> writes:
> 
> > On Thu, Oct 20, 2016 at 12:42:01PM +0200, Markus Armbruster wrote:
> >> "Dr. David Alan Gilbert" <address@hidden> writes:
> >> 
> >> > * Daniel P. Berrange (address@hidden) wrote:
> >> >> On Thu, Oct 20, 2016 at 10:55:52AM +0200, Markus Armbruster wrote:
> >> >> 
> >> >> Our code has increasingly converted to propagate errors up the call
> >> >> chain, but having a mix of different error reporting approaches
> >> >> is increasingly causing pain.
> >> >> 
> >> >> eg a function which propagates errors wants to call into a function
> >> >> whicih uses error_report
> >> 
> >> When you add an Error * parameter to a function, you get to convert
> >> everything it calls.  Failure to do so is a bug, simple as that.
> >> 
> >> Likewise, when you add a new call to a function that takes an Error *
> >> parameter.
> >
> > I agree its a bug - just that from a pragmatic POV it isn't always
> > practical to convert the entire call chain in one big bang - particularly
> > if you are working on shared infrastructure.
> 
> Been there, felt the pain.  I just wish we'd be more diligent adding
> FIXME comments when we take shortcuts.
> 
> >                                              For example, the
> > qemu-sockets.c APIs for connecting & listening on unix/tcp sockets
> > use Error * to report errors. Those APIs were used in the block
> > layer, migration, vnc, chardev and network backend code. It clearly
> > isn't practical to convert all those regions to code to correctly
> > propagate Error ** in one go. That's certainly the desired end
> > goal IMHO, but getting there is going to take a while.
> >
> > This slow conversion has been going on for a long time now, so
> > maybe we need to make an aggressive push in strategic areas of
> > the code to stop it dragging out indefinitely. This is kind of
> > a general problem we have in QEMU - we have introduced lots of
> > new standard frameworks, but very rarely finish converting
> > code to use them.
> 
> Yep.
> 
> >                    This is why I did a big push to try to get
> > everything switched over to use QIOChannel, in the shortest
> > possible time, rather than doing only small bits over many
> > releases.
> 
> Much better when you can pull it off.
> 
> A common road block is having to update code nobody wants to touch, let
> alone maintain, with no way to test.  The easy way out is to update just
> the code you actually care about, then call the conversion process
> "incremental".  Well, it's not incremental without commitments to
> increment.

I don't think we should do this until we can come up with a scheme
that has less impact on the code being modified.

> >> >>                          - there's no nice way to propagate the error
> >> >> since it has already been reported.  If the function then wants to
> >> >> explicitly ignore the error, then that's impossible too,since it has
> >> >> already been reported.  Add in our code which doesn't use error_report
> >> >> and instead returns errno values, such as the block layer, and it gets
> >> >> even worse because if that calls a function which propagates an error,
> >> >> it has to throw away that useful error and return a useless invented
> >> >> errno value :(
> >> 
> >> Different bug: when you receive an Error, you have to either handle and
> >> consume it, or pass it on.  Throwing it away and returning an error code
> >> instead counts as neither, and is a bug.
> >
> > Totally agree its a bug - just again hits the reality of having to
> > do major code conversions.
> >
> >> >> IMHO continuing to convert code to propagate errors is the only way
> >> >> out of this swamp, because it provides the greatest flexibility to
> >> >> the callers of said functions to decide how to deal with the error.
> >> 
> >> I wouldn't call the whole situation a swamp.  error_report() is just
> >> fine in many places.  So is returning -errno.  We convert to Error when
> >> these techniques cease to be fine.  The swampy part is old code where
> >> these techniques have never been fine, or at least not for a while.  To
> >> be drained incrementally.
> >
> > Realistically all the major backend subsystems (chardev, network, block,
> > ui and migration) need to be converted to Error ** propagation, since
> > they all ultimately call into some common code that reports Error **.
> 
> Infrastucture generally doesn't know how it's used, which means
> error_report() is generally wrong there.  Sufficiently simple functions
> can keep returning -errno, null, whatever, but the interesting stuff
> needs to use Error.
> > Very few places will end up being able to stick with -errno, or plain
> > error_report in the long term.
> 
> Not sure about "very few".  Less than now.  We'll see.

I'd also prefer we got the very-few level; Migration used to be
characterised by getting a 'load of migration failed -22' and having
no clue in the logs to why; I've slowly fought back to be able
to get an error from the lowest level that caused the failure.
I want more of that, so that when someone gets a rare failure in the field
I can see why.

Dave

> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK