On Thu, Sep 10, 2020 at 11:34:44AM -0400, Laine Stump wrote:
On 9/10/20 8:35 AM, Martin Kletzander wrote:On Thu, Sep 10, 2020 at 01:56:53PM +0200, Pino Toscano wrote:A lot of virReportError() calls use VIR_ERR_INTERNAL_ERROR to represent the number of the error, even in cases where there is one fitting more. Hence, replace some of them with better virErrorNumber values.This is something that we need to do in oh-so-many places, yes.I don't disagree with you, but as a tempering comment to this, I've always thought of INTERNAL_ERROR as a way to mark something that should never happen, and if it did it's due to the way the code is written, not the fault of the user. So there could be places where, e.g. yes, the argument is invalid, but the invalid argument should have been weeded out before this point, or it was selected by the software and not the user - basically it's a way of saying "This should never happen, and if it didn't, some programmer has some 'splaining to do."
Oh definitely.
I guess I'm just saying that any change would need to take into account the larger context around the error, not just the few lines where the error is detected and logged. (NB: I didn't even look at Pino's changes to see which ones he changed; just wanted to get my philosophy out there)
Similarly to me, I *think* some of them should stay, at least it looks like it, but that's why I wrote him that if it actually originates from the user, then feel free to keep the changes. So if all of them are actually related to user input errors, then: Reviewed-by: Martin Kletzander <mkletzan@xxxxxxxxxx> Or at least for those that are.
(NB2: still the most important part of any error message (for us) is knowing the exact line where the error occurred, and the values of any pertinent variables that caused it; for me the type of error (which is in many cases a touchy feely thing rather than hard fact) is secondary)
The same way the user should have an idea what they should change if this was an error on their part. I mean I myself just go over the documentation and a manual if I get something like "error: invalid argument: Invalid argument", but it might not fully reflect the state of the code.
Attachment:
signature.asc
Description: PGP signature
