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

[Dmitry V. L. <ld...@al...>]

On Fri, Sep 11, 2015 at 01:31:02PM +0200, Patrik Jakobsson wrote:
> 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?

RVAL_DECODED itself is trivial: by setting this flag parser tells its
caller that decoding is finished on entering and it shouldn't be called on
exiting of this syscall.  Setting this flag on exiting has no effect.

With regards to ioctl parsers, there might be some confusion because they
also have this unusual return code +1 semantics.  In this example, the
problem with "return RVAL_DECODED" statements is that if they happen on
exiting, it means that something has already been printed on entering
already and by returning RVAL_DECODED parser tells its caller to perform
the default action that also prints something.

That is, ioctl parser should return
- on entering:
  + any value (it's ignored) without RVAL_DECODED flag set:
    continue processing on exiting;
  + RVAL_DECODED:
    skip processing on exiting and tell the caller
    to perform default processing on entering;
  + RVAL_DECODED | (1 + RVAL_*):
    skip processing on exiting and tell the caller
    to skip default processing;
- on exiting:
  + 0 (with or without RVAL_DECODED set, it's ignored anyway):
    tell the caller to perform default processing on exiting;
  + 1 + RVAL_* (RVAL_DECODED is ignored):
    tell the caller to skip default processing.

> > > +			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.

Usally only IOW parsers return codes with RVAL_DECODED set on entering.
IOWR also print something on exiting so they shouldn't normally return
RVAL_DECODED on entering.


-- 
ldv