Messages in this thread | | | Date | Wed, 25 Nov 2020 20:25:53 +0100 | From | Borislav Petkov <> | Subject | Re: [RFC PATCH v0 03/19] x86/insn: Add an insn_decode() API |
| |
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.
https://people.kernel.org/tglx/notes-about-netiquette
| |