Re: [Xen-devel] [PATCH V6 11/13] xen/vm_event: Relocate memop checks

[Tamas Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Thu, Mar 12, 2015 at 4:36 PM, Tim Deegan <tim@xxxxxxx> wrote:

Hi,

At 01:11 +0100 on 18 Feb (1424218301), Tamas K Lengyel wrote:
> -int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpo)
> +int mem_paging_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_paging_op_t) arg)
>Â {
> -Â Â int rc = -ENODEV;
> -Â Â if ( unlikely(!d->vm_event->paging.ring_page) )
> +Â Â int rc;
> +Â Â xen_mem_paging_op_t mpo;
> +Â Â struct domain *d;
> +Â Â bool_t copyback = 0;
> +
> +Â Â rc = -EFAULT;
> +Â Â if ( copy_from_guest(&mpo, arg, 1) )
>Â Â Â Â Â return rc;

ISTR Jan suggested that you just 'return -EFAULT' here since you won't
be reusing the rc.

Ack, already fixed in v7.

Â

> -Â Â switch( mpo->op )
> +Â Â rc = rcu_lock_live_remote_domain_by_id(mpo.domain, &d);
> +Â Â if ( rc )
> +Â Â Â Â goto out;

This one should be a return too; you only need the goto out if this
succeeded.

Ack, also fixed in v7.

Â

> +Â Â rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_paging_op);
> +Â Â if ( rc )
> +Â Â Â Â goto out;
> +
> +Â Â rc = -ENODEV;
> +Â Â if ( unlikely(!d->vm_event->paging.ring_page) )
> +Â Â Â Â goto out;
> +
> +Â Â switch( mpo.op )
>Â Â Â {
>Â Â Â case XENMEM_paging_op_nominate:
> -Â Â Â Â rc = p2m_mem_paging_nominate(d, mpo->gfn);
> +Â Â Â Â rc = p2m_mem_paging_nominate(d, mpo.gfn);
>Â Â Â Â Â break;
>
>Â Â Â case XENMEM_paging_op_evict:
> -Â Â Â Â rc = p2m_mem_paging_evict(d, mpo->gfn);
> +Â Â Â Â rc = p2m_mem_paging_evict(d, mpo.gfn);
>Â Â Â Â Â break;
>
>Â Â Â case XENMEM_paging_op_prep:
> -Â Â Â Â rc = p2m_mem_paging_prep(d, mpo->gfn, mpo->buffer);
> +Â Â {

I don't think these braces are needed.

Ack.

Â

[...]
>Â Â Â /* Only HAP is supported */
> +Â Â rc = -ENODEV;
>Â Â Â if ( !hap_enabled(d) || !d->arch.hvm_domain.mem_sharing_enabled )
> -Â Â Â Â Âreturn -ENODEV;
> +Â Â Â Â Ârc = -ENODEV;
>

You need a 'goto out' here, or the rc gets overwritten immediately.

Ack, already fixed. I really need to get v7 posted =)

Â

Cheers,

Tim.

> -Â Â switch(mec->op)
> +Â Â rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_sharing_op);
> +Â Â if ( rc )
> +Â Â Â Â goto out;

Thanks,

Tamas

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