Re: [PATCH 2/5] xen/gnttab: Rework resource acquisition

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.09.2020 15:10, Andrew Cooper wrote:
> On 29/07/2020 21:02, Jan Beulich wrote:
>> On 28.07.2020 13:37, Andrew Cooper wrote:
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -4013,6 +4013,72 @@ static int gnttab_get_shared_frame_mfn(struct
>>> domain *d,
>>>       return 0;
>>>   }
>>>   +int gnttab_acquire_resource(
>>> +    struct domain *d, unsigned int id, unsigned long frame,
>>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>>> +{
>>> +    struct grant_table *gt = d->grant_table;
>>> +    unsigned int i = nr_frames, tot_frames;
>>> +    void **vaddrs;
>>> +    int rc = 0;
>>> +
>>> +    /* Input sanity. */
>>> +    if ( !nr_frames )
>>> +        return -EINVAL;
>>
>> I can't seem to be able to find an equivalent of this in the old logic,
>> and hence this looks like an unwarranted change in behavior to me. We
>> have quite a few hypercall ops where some count being zero is simply
>> a no-op, i.e. yielding success without doing anything.
> 
> There is no possible circumstance when getting here requesting 0 is
> legitimate.
> 
> Tolerating a zero input for a mapping request is a very expensive way of
> hiding caller bugs.

That's just one possible view. There are people thinking that
some extra runtime overhead doesn't outweigh the "clutter" of
extra conditionals in their code, and hence would rather avoid
checking if some calculation of theirs yielded zero before
making a hypercall. I think we should try to consistently
regard counts of zero as "nothing to do" in all our hypercalls.

> Most importantly however, the correctness of the logic does depends on
> nr_frames being nonzero.

I didn't mean to ask to drop the conditional if it's needed;
all I'm opposed to is raising an error in this case.

Jan