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

[Dmitry V. Levin]

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

pgpIAXz07kFgg.pgp
Description: PGP signature

------------------------------------------------------------------------------

_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel