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()? Cheers, Lluis