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

[Patrik Jakobsson <patrik.jakobsson@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx