Re: [PATCH v3 1/7] xen/gnttab: Rework resource acquisition

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.01.2021 17:57, Andrew Cooper wrote:
> On 15/01/2021 11:56, Jan Beulich wrote:
>>> +    /* Grow table if necessary. */
>>> +    grant_write_lock(gt);
>>> +    rc = -EINVAL;
>>> +    switch ( id )
>>> +    {
>>> +    case XENMEM_resource_grant_table_id_shared:
>>> +        vaddrs = gt->shared_raw;
>>> +        rc = gnttab_get_shared_frame_mfn(d, tot_frames - 1, &tmp);
>> ... this will degenerate (and still cause an error) when frame
>> is also zero, and will cause undue growing of the table when
>> frame is non-zero yet not overly large.
> 
> Urgh, yes - that is why I had the check.
> 
> In which case I retract my change between v2 and v3 here.
> 
>> As indicated before, I'm of the clear opinion that here - like
>> elsewhere - a number of zero frames requested means that no
>> action be taken at all, and success be returned.
> 
> The general world we work in (POSIX) agrees with my opinion over yours
> when it comes to this matter.

I assume you are referring to mmap()? I ask because I think there
are numerous counter examples (some even in the C standard):
malloc() & friends allow for either behavior. memcpy() / memmove()
happily do nothing when passed a zero size. read() / write()
are at least allowed to read/write nothing (and return success)
when told so. Otoh I notice that a zero vector count passed to
readv() / writev() is indeed an error, yet nothing is said at all
about individual vector elements specifying zero size.

Plus of course I don't think POSIX is the main reference point
here, when the rest of the hypercalls allowing for some form of
batching permit empty batches.

> I spent a lot of time and effort getting this logic correct in v2, and I
> do not have any further time to waste adding complexity to support a
> non-existent corner case, nor is it reasonable to further delay all the
> work which is depending on this series.  This entire mess is already too
> damn complicated, without taking extra complexity.
> 
> Entertaining the idea of supporting 0 length requests is really not as
> simple as you seem to think it is, and is a large part of why I'm
> stubbornly refusing to do so.

I'd be really happy to be educated of the complications; sadly
so far you've only claimed ones would exist without actually
going into sufficient detail. In particular I don't view placing 

    if ( size == 0 )
        return 0;

suitably early coming anywhere near "complexity". Even more so
that as per your reply you mean to undo removal of a respective
check, just that in your version it'll return an error instead
of success.

> I am going to commit this patch (with some of the other minor adjustments).

I'm not concerned enough of the introduced inconsistency to
outright veto you doing so, but I still don't think this is an
appropriate step to take under the present conditions.

Jan