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

From: George Dunlap <george.dunlap@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Konrad Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Ian Jackson <ian.jackson@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH v3] CODING_STYLE: Document how to handle unexpected conditions
Date: Mon, 6 Jan 2020 16:19:46 +0000	[thread overview]
Message-ID: <500f6dd1-b36f-c600-78c3-60d0f94123a0@citrix.com> (raw)
In-Reply-To: <49c55a7a-af65-9b1c-c5db-1664571a4393@suse.com>

On 12/10/19 1:46 PM, Jan Beulich wrote:
> On 10.12.2019 11:56, George Dunlap wrote:
>> On 12/9/19 1:50 PM, Jan Beulich wrote:
>>> On 09.12.2019 12:29, George Dunlap wrote:
>>>> --- a/CODING_STYLE
>>>> +++ b/CODING_STYLE
>>>> @@ -133,3 +133,97 @@ the end of files.  It should be:
>>>>   * indent-tabs-mode: nil
>>>>   * End:
>>>>   */
>>>> +
>>>> +Handling unexpected conditions
>>>> +------------------------------
>>>> +
>>>> +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 isn't too hard to fix.
>>>> +
>>>> +domain_crash() should be used when passing errors up the stack is too
>>>> +difficult, and/or when fixing up state of a guest is impractical, but
>>>> +where fixing up the state of Xen will allow Xen to continue running.
>>>> +This is particularly appropriate when the guest is exhibiting behavior
>>>> +well-behaved guest should.
>>>
>>> DYM "shouldn't"?
>>
>> Indeed.
> 
> (Btw, noticing only now - there's also either an "a" missing, or it
> wants to be "guests".)
> 
>>>> +- domain_crash() is similar to BUG_ON(), but with a more limited
>>>> +effect: it stops that domain immediately.  In situations where
>>>> +continuing might cause guest or hypervisor corruption, but destroying
>>>> +the guest allows the hypervisor to continue, this can change a more
>>>> +serious bug into a guest denial-of-service.  But in situations where
>>>> +returning an error might be safe, then domain_crash() can change a
>>>> +benign failure into a guest denial-of-service.
>>>
>>> Perhaps further put emphasis on the call tree still getting unwound
>>> normally, which may imply further actions on the (now dying) domain
>>> taken. Unfortunately it's not unusual for people to forget this; I
>>> think the IOMMU code in particular was (hopefully isn't so much
>>> anymore) a "good" example of this.
>>
>> Can you expand on this?  Do you mean to advise that care should be taken
>> when returning up the callstack that the domain which was running before
>> may now be dying, and to behave appropriately?
> 
> One issue is with functions returning void, where the caller won't
> even know that something may have gone wrong. Another is that
> typically error paths are less commonly used, and crashing a
> domain would typically be accompanied by indicating an error to
> the upper layers. Hence such crashing may trigger unrelated bugs.
> A third aspect is that, indeed, dying guests may need special
> treatment (see the already existing ->is_dying checks we have).
> 
> I mentioned the call tree unwinding in particular because earlier
> on we had domain_crash_synchronous(), which was there specifically
> to avoid issues with errors (and the changed state) bubbling back
> up. But this model had other issues, hence our movement away from
> it.

How about a paragraph like this:

---
Note however that domain_crash() has its own traps: callers far up the
call stack may not realize that the domain is now dying as a result of
an innocuous-looking operation, particularly if somewhere between the
failure and the operation, no error is returned.  Using it requires
careful inspection and documentation of the code to make sure all
callers at the stack handle a newly-dead domain gracefully.
---

 -George

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