Re: [Intel-gfx] [PATCH v4 4/5] drm: Add decoding of i915 ioctls

[Patrik Jakobsson]

On Tue, Sep 08, 2015 at 04:18:11AM +0300, Dmitry V. Levin wrote:
> On Mon, Aug 24, 2015 at 02:42:49PM +0200, Patrik Jakobsson wrote:
> > +static int i915_getparam(struct tcb *tcp, const unsigned int code, long 
> > arg)
> > +{
> > +   struct drm_i915_getparam param;
> > +   int value;
> > +
> > +   if (umove(tcp, arg, &param))
> > +           return RVAL_DECODED;
> > +
> > +   if (entering(tcp)) {
> > +           tprints(", {param=");
> > +           printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
> > +   } else if (exiting(tcp)) {
> > +           if (umove(tcp, (long)param.value, &value))
> > +                   return RVAL_DECODED;
> 
> Since part of param has already been printed, RVAL_DECODED shouldn't be
> returned here.  For the same reason, RVAL_DECODED shouldn't be returned
> earlier in this function.
> 

The usage of RVAL_DECODED is quite confusing. RVAL_DECODED to me should be "We
decoded this don't do any fallback". How did you intent it to be used?

> > +           tprints(", value=");
> > +           switch (param.param) {
> > +           case I915_PARAM_CHIPSET_ID:
> > +                   tprintf("0x%04x", value);
> 
> Since the value has been fetched by address stored in param.value,
> it has to be printed in brackets like in printnum_int.

Ok

> > +                   break;
> > +           default:
> > +                   tprintf("%d", value);
> 
> Likewise.
> 
> > +           }
> > +           tprints("}");
> > +   }
> > +
> > +   return RVAL_DECODED | 1;
> 
> This shouldn't be returned on entering(tcp).

If all decoding would be done when entering is finished, wouldn't it make sense
to allow RVAL_DECODED here? Still don't understand how this is intended to work.

> > +}
> 
> So the whole function should look smth like this:
> 
> static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
> {
>       struct drm_i915_getparam param;
> 
>       if (entering(tcp)) {
>               if (umove_or_printaddr(tcp, arg, &param))
>                       return RVAL_DECODED | 1;
>               tprints(", {param=");
>               printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
>               tprints(", value=");
>               return 0;
>       } else {
>               int value;
> 
>               if (umove(tcp, arg, &param)) {
>                       tprints("???");
>               } else if (!umove_or_printaddr(tcp, (long) param.value, 
> &value)) {
>                       switch (param.param) {
>                       case I915_PARAM_CHIPSET_ID:
>                               tprintf("[%#04x]", value);
>                               break;
>                       default:
>                               tprintf("[%d]", value);
>                       }
>               }
>               tprints("}");
>               return 1;
>       }
> }
> 
> Please apply this approach to all DRM_IOWR parsers.
> 
> > +
> > +static int i915_setparam(struct tcb *tcp, const unsigned int code, long 
> > arg)
> > +{
> > +   struct drm_i915_setparam param;
> > +
> > +   if (entering(tcp)) {
> > +           if (umove(tcp, arg, &param))
> > +                   return RVAL_DECODED;
> 
> In this and other functions I slightly prefer
>       if (umove_or_printaddr(tcp, arg, &param))
>               return RVAL_DECODED | 1;
> over your variant because umove_or_printaddr() handles NULL address
> and !verbose(tcp) case better.

Agreed

> 
> 
> -- 
> ldv


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx