Messages in this thread | | | Date | Sat, 24 Oct 2020 16:13:15 +0900 | From | Masami Hiramatsu <> | Subject | Re: [RFC] Have insn decoder functions return success/failure |
| |
On Sat, 24 Oct 2020 01:27:41 +0200 Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Oct 23, 2020 at 07:47:04PM +0900, Masami Hiramatsu wrote: > > Thanks! I look forward to it. > > Ok, here's a first stab, it is a single big diff and totally untested > but it should show what I mean. I've made some notes while converting, > as I went along.
Thanks, so will you split this into several patches, since I saw some cleanups in this patch?
> > Have a look at insn_decode() and its call sites: they are almost trivial > now because caller needs simply to do: > > if (insn_decode(insn, buffer, ...)) > > and not care about any helper functions.
Yeah, that's good to me because in the most cases, user needs prefix, length or total decoded info.
BTW, it seems you returns 1 for errors, I rather like -EINVAL or -EILSEQ for errors so that user can also write
if (insn_decode() < 0) ...
I think "positive" and "zero" pair can easily mislead user to "true" and "false" trap.
> For some of the call sites it still makes sense to do a piecemeal insn > decoding and I've left them this way but they can be converted too, if > one wants.
Yeah, for the kprobes, if you see the insn_init() and insn_get_length() those can be replaced with one insn_decode().
> In any case, just have a look please and lemme know if that looks OKish. > I'll do the actual splitting and testing afterwards.
Except for the return value, it looks good to me.
Thank you,
-- Masami Hiramatsu <mhiramat@kernel.org>
| |