Re: [Xen-devel] [PATCH for-next] CODING_STYLE: Document how to handle unexpected conditions

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 3/1/19 5:12 PM, Jan Beulich wrote:
>>>> George Dunlap <george.dunlap@xxxxxxxxxx> 02/28/19 7:50 PM >>>
>> +GUIDELINES:
>> +
>> +Passing errors up the stack should be used when the caller is already
>> +expecting to handle errors, and the state when the error was
>> +discovered isn’t broken, or too hard to fix.
>> +
>> +BUG_ON() should be used when you can’t pass errors up the stack, and
>> +continuing would certainly cause a vulnerability.
>> +
>> +ASSERT() should be used when continuing might work, or might have an
>> +effect later whose badness is equal or less than that of a host crash;
>> +OR whose truth can be clearly observed from the code directly
>> +surrounding it.  In particular, using an ASSERT() along with returning
>> +an error code, when suitable, is a helpful pattern for finding
>> +violations of assumptions during testing, but minimizing impact on
>> +production hypervisors.
> 
> Along with these I strongly think the option of domain_crash() ought to be
> listed, and it should always be preferred over BUG_ON() as long as context
> permits.

Ah, right, I totally forgot about this case.  I'll revise and add this in.

>> +RATIONALE:
>> +
>> +It's frequently the case that code is writen with the assumption that
>> +certain conditions can never happen.  There are several possible
>> +actions programmers can take in these situations:
>> +
>> +* Programmers can simply not handle those cases in any way, other than
>> +perhaps to write a comment documenting what the assumption is.
>> +
>> +* Programmers can try to handle the case gracefully -- fixing up
>> +in-progress state and returning an error to the user.
>> +
>> +* Programmers can use ASSERT(), which will cause the check to be
>> +executed in DEBUG builds, and cause the hypervisor to crash if it's
>> +violated
> 
> Is it perhaps worth calling out explicitly that the supposed crash may occur
> much later, in a different context, and hence be perhaps rather difficult to
> analyze/debug?

Sorry, I don't quite understand this -- when you trigger an ASSERT() it
crashes right away last time I checked.  Did you mean instead to reply
to the ASSERT() section of the GUIDELINES, which says you can use ASSERT
if it may have an effect later whose badness is equal to or less than a
host crash?  Or did you mean I should call out that ASSERT()s can be
used to catch errors closer to the source, rather than later?

It also occurs to me that ASSERT()s are really orthogonal to the other
three: At each point, you should consider whether in a production
hypervisor you should 1) do nothing, 2) return an error, 3) crash the
domain, or 4) crash the hypervisor; and in the case of 1-3, you might
also want to add an ASSERT to move the detection of unexpected state
closer to the point where it happens.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel