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

["Dmitry V. Levin" <ldv@xxxxxxxxxxxx>]

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.

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

> +			break;
> +		default:
> +			tprintf("%d", value);

Likewise.

> +		}
> +		tprints("}");
> +	}
> +
> +	return RVAL_DECODED | 1;

This shouldn't be returned on entering(tcp).

> +}

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.


-- 
ldv

Attachment: pgpXEnlBO_h3r.pgp
Description: PGP signature

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