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

Re: [libvirt PATCH] esx: improve some of the virErrorNumber used



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 redhat com>

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


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