On 01.04.2021 15:20, Andrew Cooper wrote:
> On 31/03/2021 15:03, Jan Beulich wrote:
>> On 31.03.2021 15:31, Andrew Cooper wrote:
>>> vlapic_init()'s caller calls vlapic_destroy() on error. Therefore, the
>>> error
>>> path from __map_domain_page_global() failing would doubly free
>>> vlapic->regs_page.
>> I'm having difficulty seeing this. What I find at present is
>>
>> rc = vlapic_init(v);
>> if ( rc != 0 ) /* teardown: vlapic_destroy */
>> goto fail2;
>>
>> and then
>>
>> fail3:
>> vlapic_destroy(v);
>> fail2:
>>
>> Am I missing some important aspect?
>
> No - I'm blind. (although I do blame the code comment for being
> actively misleading.)
>
> I retract the patch at this point. It needs to be part of a bigger
> series making changes like this consistently across the callers.
>
>>> Rework vlapic_destroy() to be properly idempotent, introducing the necessary
>>> UNMAP_DOMAIN_PAGE_GLOBAL() and FREE_DOMHEAP_PAGE() wrappers.
>>>
>>> Rearrange vlapic_init() to group all trivial initialisation, and leave all
>>> cleanup to the caller, in line with our longer term plans.
>> Cleanup functions becoming idempotent is what I understand is the
>> longer term plan. I didn't think this necessarily included leaving
>> cleanup after failure in a function to it caller(s).
>
> That's literally the point of the exercise.
>
>> At least in the
>> general case I think it would be quite a bit better if functions
>> cleaned up after themselves - perhaps (using the example here) by
>> vlapic_init() calling vlapic_destroy() in such a case.
>
> That pattern is the cause of code duplication (not a problem per say),
> and many bugs (the problem needing solving) caused by the duplicated
> logic not being equivalent.
>
> We've got the start of the top-level pattern in progress, with
> {domain,vcpu}_create() calling {d,v}_teardown() then {d,v}_destroy() for
> errors.
Hmm, in which case you mean to shift the responsibility not to "the
caller" (many instances) but "the top level caller" (a single
instance)?
Jan
