Re: [PATCH Altp2m cleanup v5 1/3] altp2m cleanup work.

["Lai, Paul" <paul.c.lai@intel.com>]

On Mon, Sep 26, 2016 at 10:01:50AM -0600, Jan Beulich wrote:
> >>> On 13.09.16 at 18:46, <paul.c.lai@intel.com> wrote:
> > Indent goto labels by one space.
> > Inline (header) altp2m functions.
> > In do_altp2m_op(), during the sanity check of the passed command,
> > return -ENONSYS if not a valid command.
> > In do_altp2m_op(), when evaluating a command, ASSERT_UNREACHABLE()
> > if the command is not recognizable.  The sanity check above should
> > have triggered the return of -ENOSYS.
> > Make altp2m_vcpu_emulate_ve() return actual bool_t (rather than return
> > void()).
> > 
> > Signed-off-by: Paul Lai <paul.c.lai@intel.com>
> > ---
> 
> It is very disappointing to find that there is still no information here
> on what changed from the previous version.
> 
> > @@ -5349,6 +5362,8 @@ static int do_altp2m_op(
> >              rc = p2m_change_altp2m_gfn(d, a.u.change_gfn.view,
> >                      _gfn(a.u.change_gfn.old_gfn),
> >                      _gfn(a.u.change_gfn.new_gfn));
> > +    default:
> > +        ASSERT_UNREACHABLE();
> >      }
> 
> And it is even worse that the bug pointed out here is still present.

I reread the comment in 
   https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg00937.html
and missed the "unintended fallthrough".
Will correct.

> 
> >  /* emulates #VE */
> > -bool_t altp2m_vcpu_emulate_ve(struct vcpu *v);
> > +static inline bool_t altp2m_vcpu_emulate_ve(struct vcpu *v)
> 
> Nor did you switch to plain bool here, as was requested during both
> v3 and v4 review.

I did not know "bool" existed, and thought "plain bool" was "bool_t".
I have looked around and see that "bool" is better defined ({0,1} instead of
{0, !0}) and hopefully understand your motivation for using/requiring it.
> 
> Please do not resubmit this series until you have taken care of
> _all_ review comments you've received so far. As with the
> previous version I'm not going to spend time looking at the other
> two patches due to this fundamental requirement, despite
> having been pointed out before, not being met.
> 
> Jan
> 

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