From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Dmitry V. Levin" <ldv@altlinux.org>
Subject: Re: [PATCH v4 4/5] drm: Add decoding of i915 ioctls
Date: Tue, 8 Sep 2015 04:18:11 +0300
Message-ID: <20150908011811.GB17781@altlinux.org>
References: <1440420170-13337-1-git-send-email-patrik.jakobsson@linux.intel.com>
 <1440420170-13337-5-git-send-email-patrik.jakobsson@linux.intel.com>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============1603659869=="
Return-path: <intel-gfx-bounces@lists.freedesktop.org>
Received: from pegasus3.altlinux.org (pegasus3.altlinux.org [194.107.17.103])
 by gabe.freedesktop.org (Postfix) with ESMTP id B597C6E417
 for <intel-gfx@lists.freedesktop.org>; Mon,  7 Sep 2015 18:18:13 -0700 (PDT)
In-Reply-To: <1440420170-13337-5-git-send-email-patrik.jakobsson@linux.intel.com>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
 <mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
 <mailto:intel-gfx-request@lists.freedesktop.org?subject=subscribe>
Errors-To: intel-gfx-bounces@lists.freedesktop.org
Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>
To: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, strace-devel@lists.sourceforge.net
List-Id: intel-gfx@lists.freedesktop.org


--===============1603659869==
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature"; boundary="8P1HSweYDcXXzwPJ"
Content-Disposition: inline


--8P1HSweYDcXXzwPJ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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=3D");
> +		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=3D");
> +		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=3D");
		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
		tprints(", value=3D");
		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.


--=20
ldv

--8P1HSweYDcXXzwPJ
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iEYEARECAAYFAlXuN1MACgkQfKvmrJ41Nh7z3gCcC/FV/XOEh99S1nfbP0IZCDvm
WNQAoMSOURRq+o7aJcTd3uxu/zqHNaXk
=7pn5
-----END PGP SIGNATURE-----

--8P1HSweYDcXXzwPJ--

--===============1603659869==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4
IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0
cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK

--===============1603659869==--