On Tue, Nov 12, 2019 at 4:54 AM Jan Beulich <jbeul...@suse.com> wrote:
>
> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> > @@ -4681,7 +4682,7 @@ static int do_altp2m_op(
> > break;
> >
> > case HVMOP_altp2m_set_suppress_ve:
> > - if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> > + if ( a.u.suppress_ve.pad1 )
>
> Just because the field changes its name doesn't mean you can
> drop the check. You even add a new field not used (yet) by
> this sub-function, which then also would need checking here.
>
> > @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
> > }
> > break;
> >
> > + case HVMOP_altp2m_set_suppress_ve_multi:
> > + if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
>
> A count of zero typically is taken as a no-op, not an error.
>
> > + rc = -EINVAL;
> > + else
> > + {
> > + rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
> > +
> > + if ( rc == -ERESTART )
> > + if ( __copy_field_to_guest(guest_handle_cast(arg,
> > + xen_hvm_altp2m_op_t),
> > + &a, u.suppress_ve.opaque) )
> > + rc = -EFAULT;
>
> If the operation is best effort, _some_ indication of failure should
> still be handed back to the caller. Whether that's through the opaque
> field or by some other means is secondary. If not via that field
> (which would make the outer of the two if()-s disappear), please fold
> the if()-s.
At least for mem_sharing_range_op we also do a best-effort and don't
return an error for pages where it wasn't possible to share. So I
don't think it's absolutely necessary to do that, especially if the
caller can't do anything about those errors anyway.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
