Thomas Huth writes: > On 27.01.2016 20:20, Lluís Vilanova wrote: >> Lluís Vilanova writes: >> >>> Thomas Huth writes: >>>> On 20.01.2016 15:10, Lluís Vilanova wrote: >>>>> Thomas Huth writes: >>>>> >>>>>> On 18.01.2016 21:26, Eric Blake wrote: >>>>>>> On 01/15/2016 06:54 AM, Lluís Vilanova wrote: >>>>>>>> Gives some general guidelines for reporting errors in QEMU. >>>>>>>> >>>>>>>> Signed-off-by: Lluís Vilanova <vilan...@ac.upc.edu> >>>>>>>> --- >>>>>>>> HACKING | 36 ++++++++++++++++++++++++++++++++++++ >>>>>>>> 1 file changed, 36 insertions(+) >>>>>> ... >>>>>>>> +Functions in this header are used to accumulate error messages in an >>>>>>>> 'Error' >>>>>>>> +object, which can be propagated up the call chain where it is finally >>>>>>>> reported. >>>>>>>> + >>>>>>>> +In its simplest form, you can immediately report an error with: >>>>>>>> + >>>>>>>> + error_setg(&error_fatal, "Error with %s", "arguments"); >>>>>>> >>>>>>> This paradigm doesn't appear anywhere in the current code base >>>>>>> (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but >>>>>>> nothing directly passes error_fatal). It's a bit odd to document >>>>>>> something that isn't actually used. >>>>> >>>>>> +1 for _not_ documenting this here: IMHO this looks ugly. If we want >>>>>> something like this, I think we should introduce a proper >>>>>> error_report_fatal() function instead. >>>>> >>>>> That's a bit of a chicken and egg problem. My main intention was to >>>>> provide a >>>>> best practices summary on reporting messages/errors, since QEMU's code is >>>>> really >>>>> heterogeneous on that regard. But there seems to be no consensus on some >>>>> details >>>>> of what the proper way should be with the current interfaces. >>>>> >>>>> Utility functions for "regular messages", warnings, fatals and aborts >>>>> would >>>>> definitiely be an improvement IMHO, but I dont have time to adapt >>>>> existing code >>>>> to these (and I was told not to add unused utility functions for this). >>>>> >>>>> Now, if I were able to add such functions, it'd be something like: >>>>> >>>>> // Generate message "as is"; not sure if this should exist. >>>>> message_report(fmt, ...) >> >>>> Not sure what this should be good for? We've already got error_report() >>>> that generates messages "as is", haven't we? >> >>> Well, it just seemed wrong to me using error_report() to report "regular >>> messages" :) >> >> >>>>> // Generate message with prepended file/line information for the caller. >>>>> // Calls exit/abort on the last two. >>>>> error_report_{warn,fatal,abort}(fmt, ...) >>>>> >>>>> // Same with an added message from strerror. >>>>> error_report_{warn,fatal,abort}_errno(fmt, ...) >>>>> >>>>> But, should I add these without providing extensive patches that refactor >>>>> code >>>>> to use them? >> >>>> Maybe create a patch that introduces the _fatal and _abort functions >>>> (I'd skip the _warn functions for now), and use them in one or two files >>>> (e.g. replace the error_setg(&error_abort, ...) in spapr.c). That should >>>> not be that much of work, and could be a good base for further discussion? >> >>> I can do that. But then should 'error_fatal' and 'error_abort' be officially >>> deprecated in favour of error_report_fatal() and error_report_abort()? >> >> Sorry, I see this is misleading. I mean deprecate directly using >> "error_setg(error_fatal)"; you can still decide to pass error_fatal as an >> error >> object to other user functions.
> Since we hardly got any of these in the code right now, I don't see an > urgent need to explicitely say that this should be deprecated. I hope > that people rather will use the new functions automatically instead > since these sounds much more intuitive, IMHO. Got it. I'm writing some of them now. Cheers, Lluis