Re: [RFC PATCH v0 03/19] x86/insn: Add an insn_decode() API

Borislav Petkov
2020-11-25T19:26:19Z

On Thu, Nov 26, 2020 at 01:53:33AM +0900, Masami Hiramatsu wrote:
> (only from the viewpoint of VEX coding, a bit stricter, but not perfect.)

Yeah, I wanted to document the fact that it has changed behavior in the
commit message - we'll make it perfect later. :-)

> > @@ -98,8 +101,12 @@ static void insn_get_emulate_prefix(struct insn *insn)
> > * Populates the @insn->prefixes bitmap, and updates @insn->next_byte
> > * to point to the (first) opcode. No effect if @insn->prefixes.got
> > * is already set.
> > + *
> > + * * Returns:
> > + * 0: on success
> > + * !0: on error
> > */
>
> So this is different from...
>
> [..]
> > +
> > +/**
> > + * insn_decode() - Decode an x86 instruction
> > + * @insn: &struct insn to be initialized
> > + * @kaddr: address (in kernel memory) of instruction (or copy thereof)
> > + * @buf_len: length of the insn buffer at @kaddr
> > + * @m: insn mode, see enum insn_mode
> > + *
> > + * Returns:
> > + * 0: if decoding succeeded
> > + * < 0: otherwise.
>
> this return value.
>
> Even for the insn_get_*(), I would like to see them returning -EINVAL
> as same as insn_decode(). Same API group has different return value is
> confusing.

Right, my goal in the end here is to make *all* users of the decoder
call insn_decode() and nothing else. And there you can have different
return values so checking negative/positive is the proper way to go.

Those other helpers, though, should then become internal and for those I
think it is easier to use them when they return a boolean yes/no value,
meaning, they do one thing and one thing only:

For example, it is more readable to do:

if (insn_...)

vs

int ret;

...

ret = insn_,...()
if (ret)
...

which is 4 more lines of error handling and return variable, leading to
more code.

But if you want to be able to use those other helpers outside of the
decoder - for whatever reason - then sure, the function signatures
should be the same.

Thoughts?

--
Regards/Gruss,
Boris.