Re: [PATCH 3/5] xen/memory: Fix compat XENMEM_acquire_resource for size requests

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/07/2020 21:09, Jan Beulich wrote:
> On 28.07.2020 13:37, Andrew Cooper wrote:
>> Copy the nr_frames from the correct structure, so the caller doesn't
>> unconditionally receive 0.
>
> Well, no - it does get copied from the correct structure. It's just
> that the field doesn't get set properly up front.

You appear to be objecting to my use of the term "correct".

There are two structures.  One contains the correct value, and one
contains the wrong value, which happens to always be 0.

I stand by sentence as currently written.

> Otherwise you'll
> (a) build in an unchecked assumption that the native and compat
> fields match in type

Did you actually check?  Because I did before embarking on this course
of action.

In file included from /local/xen.git/xen/include/xen/guest_access.h:10:0,
                 from compat/memory.c:5:
/local/xen.git/xen/include/asm/guest_access.h:152:28: error: comparison
of distinct pointer types lacks a cast [-Werror]
     (void)(&(hnd).p->field == _s);                      \
                            ^
compat/memory.c:628:22: note: in expansion of macro ‘__copy_field_to_guest’
                 if ( __copy_field_to_guest(
                      ^~~~~~~~~~~~~~~~~~~~~

This is what the compiler thinks of the code, when nr_frames is changed
from uint32_t to unsigned long.


> and (b) set a bad example for people looking
> here

This entire function is a massive set of bad examples; the worst IMO
being the fact that there isn't a single useful comment anywhere in it
concerning how the higher level loop structure works.

I'm constantly annoyed that I need to reverse engineer it from scratch
every time I look at it, despite having a better-than-most understanding
of what it is trying to achieve, and how it is supposed to work.

I realise this is noones fault in particular, but it is not
fair/reasonable to claim that this change is the thing setting a bad
example in this file.

> and then cloning this code in perhaps a case where (a) is not
> even true. If you agree, the alternative change of setting
> cmp.mar.nr_frames from nat.mar->nr_frames before the call is

Is there more to this sentence?

While this example could be implemented (at even higher overhead) by
copying nat back to cmp before passing it back to the guest, the same is
not true for the changes required to fix batching (which is another
series the same size as this).

~Andrew