Re: [Xen-devel] [RFC PATCH V2 3/8] xen/mem_paging: Convert mem_event_op to mem_paging_op

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Thu, Jan 22, 2015 at 4:09 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 18.01.15 at 16:17, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/mm/mem_paging.c
>> +++ b/xen/arch/x86/mm/mem_paging.c
>> @@ -25,31 +25,31 @@
>>  #include <xen/mem_event.h>
>>
>>
>> -int mem_paging_memop(struct domain *d, xen_mem_event_op_t *mec)
>> +int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpc)
>>  {
>>      if ( unlikely(!d->mem_event->paging.ring_page) )
>>          return -ENODEV;
>>
>> -    switch( mec->op )
>> +    switch( mpc->op )
>>      {
>>      case XENMEM_paging_op_nominate:
>>      {
>> -        unsigned long gfn = mec->gfn;
>> +        unsigned long gfn = mpc->gfn;
>>          return p2m_mem_paging_nominate(d, gfn);
>
> If you fiddle with this, please fix obvious style issues - here, blank
> line after declaration(s). Albeit in the particular cases here I wonder
> whether the local variable is really helpful.

Ack.

>
>> --- a/xen/common/mem_event.c
>> +++ b/xen/common/mem_event.c
>> @@ -474,7 +474,7 @@ int do_mem_event_op(int op, uint32_t domain, void *arg)
>>      {
>>  #ifdef HAS_MEM_PAGING
>>          case XENMEM_paging_op:
>> -            ret = mem_paging_memop(d, (xen_mem_event_op_t *) arg);
>> +            ret = mem_paging_memop(d, (xen_mem_paging_op_t *) arg);
>
> Afaict the cast is useless and should be dropped rather than replaced.

Ack.

>
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -372,7 +372,7 @@ typedef struct xen_pod_target xen_pod_target_t;
>>  #define XENMEM_paging_op_evict              1
>>  #define XENMEM_paging_op_prep               2
>>
>> -struct xen_mem_event_op {
>> +struct xen_mem_paging_op {
>>      uint8_t     op;         /* XENMEM_*_op_* */
>
> Especially this comment makes me think this was originally intended
> for more than just paging. Are we really determined for this to no
> longer be the case? If so, the comment should be updated too.

Ack. The only thing I could imagine is that sharing was intended to be
moved over to use this, but that never happened.

Tamas

>
> Jan
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel