Re: [PATCH 1 of 7] Tools: Sanitize mem_event/access/paging interfaces

["Andres Lagar-Cavilla" <andres@lagarcavilla.org>]

> On Thu, 2012-02-23 at 06:05 +0000, Andres Lagar-Cavilla wrote:
>> tools/libxc/xc_mem_access.c         |  10 ++++++++--
>>  tools/libxc/xc_mem_event.c          |  13 ++++++++-----
>>  tools/libxc/xc_mem_paging.c         |  10 ++++++++--
>>  tools/libxc/xenctrl.h               |   6 +++---
>>  tools/tests/xen-access/xen-access.c |  22 ++++------------------
>>  tools/xenpaging/xenpaging.c         |  18 +++---------------
>>  tools/xenpaging/xenpaging.h         |   2 +-
>>  xen/arch/x86/mm/mem_event.c         |  33
>> +++------------------------------
>>  xen/include/public/domctl.h         |   4 ++--
>>  xen/include/public/mem_event.h      |   4 ----
>>  xen/include/xen/sched.h             |   2 --
>>  11 files changed, 40 insertions(+), 84 deletions(-)
>>
>>
>> Don't use the superfluous shared page, return the event channel directly
>> as
>> part of the domctl struct, instead.
>>
>> In-tree consumers (xenpaging, xen-access) updated. This is an ABI/API
>> change,
>> so please voice any concerns.
>>
>> Known pending issues:
>> - pager could die and its ring page could be used by some other process,
>> yet
>>   Xen retains the mapping to it.
>> - use a saner interface for the paging_load buffer.
>>
>> This change also affects the x86/mm bits in the hypervisor that process
>> the
>> mem_event setup domctl.
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>>
>> diff -r 8ddd13cc783e -r 3b24188d8815 tools/libxc/xc_mem_access.c
>> --- a/tools/libxc/xc_mem_access.c
>> +++ b/tools/libxc/xc_mem_access.c
>> @@ -25,12 +25,18 @@
>>
>>
>>  int xc_mem_access_enable(xc_interface *xch, domid_t domain_id,
>> -                        void *shared_page, void *ring_page)
>> +                         uint32_t *port, void *ring_page)
>>  {
>> +    if ( !port )
>> +    {
>> +        errno = -EINVAL;
>
> Aren't errno vals normally +ve?
>
> Is there any situation where port==NULL would be valid, i.e. any chance
> that this might happen, or are such callers just buggy?

Definitely, it should be errno = EINVAL. I'll fix this in this patch and
the sharing ring one.
>
>> +        return -1;
>> +    }
>
>> +
>>      return xc_mem_event_control(xch, domain_id,
>>                                  XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE,
>>                                  XEN_DOMCTL_MEM_EVENT_OP_ACCESS,
>> -                                shared_page, ring_page);
>> +                                port, ring_page);
>>  }
>>
>>  int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)
>> diff -r 8ddd13cc783e -r 3b24188d8815 tools/libxc/xc_mem_event.c
>> --- a/tools/libxc/xc_mem_event.c
>> +++ b/tools/libxc/xc_mem_event.c
>> @@ -24,19 +24,22 @@
>>  #include "xc_private.h"
>>
>>  int xc_mem_event_control(xc_interface *xch, domid_t domain_id, unsigned
>> int op,
>> -                         unsigned int mode, void *page, void
>> *ring_page)
>> +                         unsigned int mode, uint32_t *port, void
>> *ring_page)
>>  {
>>      DECLARE_DOMCTL;
>> +    int rc;
>>
>>      domctl.cmd = XEN_DOMCTL_mem_event_op;
>>      domctl.domain = domain_id;
>>      domctl.u.mem_event_op.op = op;
>>      domctl.u.mem_event_op.mode = mode;
>> -
>> -    domctl.u.mem_event_op.shared_addr = (unsigned long)page;
>> -    domctl.u.mem_event_op.ring_addr = (unsigned long)ring_page;
>> +    domctl.u.mem_event_op.ring_addr = (unsigned long) ring_page;
>>
>> -    return do_domctl(xch, &domctl);
>> +    errno = 0;
>> +    rc = do_domctl(xch, &domctl);
>> +    if ( !rc && port )
>> +        *port = domctl.u.mem_event_op.port;
>> +    return rc;
>
> Clearing errno is an unusual pattern, normally callers only expect errno
> to contain a valid value on failure. Are you trying to provide some
> backwards compatibility with something or is there another reason for
> doing it this way?

I can remove that as well.
Thanks,
Andres
>
>>  }
>>
>>  int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
>> diff -r 8ddd13cc783e -r 3b24188d8815 tools/libxc/xc_mem_paging.c
>> --- a/tools/libxc/xc_mem_paging.c
>> +++ b/tools/libxc/xc_mem_paging.c
>> @@ -25,12 +25,18 @@
>>
>>
>>  int xc_mem_paging_enable(xc_interface *xch, domid_t domain_id,
>> -                        void *shared_page, void *ring_page)
>> +                         uint32_t *port, void *ring_page)
>>  {
>> +    if ( !port )
>> +    {
>> +        errno = -EINVAL;
>
> Same comments as before.
>
>> +        return -1;
>> +    }
>> +
>>      return xc_mem_event_control(xch, domain_id,
>>                                  XEN_DOMCTL_MEM_EVENT_OP_PAGING_ENABLE,
>>                                  XEN_DOMCTL_MEM_EVENT_OP_PAGING,
>> -                                shared_page, ring_page);
>> +                                port, ring_page);
>>  }
>>
>
> Ian.
>
>
>