> On Fri, 2022-06-03 at 14:31 -0700, Song Liu wrote:
> > On Fri, Jun 3, 2022 at 7:11 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> > +#define INSN_OFF_MASK ((s16)0xFFFF)
> > +#define INSN_IMM_MASK ((s32)0xFFFFFFFF)
>
> Shall we use __s16 and __s32 to match struct bpf_insn exactly.
Will do.
[...]
> > + __u32 buf_elt_size = sizeof(**buf);
>
> I guess elt means "element"? I would recommend use sizeof(struct bpf_insn)
> directly.
Will do.
[...]
> > +static int null_terminated_insn_len(struct bpf_insn *seq, int max_len)
> > +{
> > + for (int i = 0; i < max_len; ++i) {
>
> Sorry for missing this in v1. We should really pull variable
> declaration out, like
>
> int i;
>
> for (int i = 0; ...)
Sorry, just to clarify, you want me to pull all loop counter
declarations to the top of the relevant functions, right? (Affecting 4
functions in this patch).
[...]
> > +static int find_insn_subseq(struct bpf_insn *seq, struct bpf_insn *subseq,
> > + if (check_unexpected &&
> > + find_all_insn_subseqs(buf, test->unexpected_insns,
> > + cnt, MAX_UNEXPECTED_INSNS)) {
>
> I wonder whether we want different logic for unexpected_insns. With multiple
> sub sequences, say seq-A and seq-B, it is more natural to reject any results
> with either seq-A or seq-B. However, current logic will reject seq-A => seq-B,
> but will accept seq-B => seq-A. Does this make sense?
Have no strong opinion on this topic. In theory different variants
might be useful in different cases.
In the test cases for bpf_loop inlining I only had to match a single
unexpected instruction, so I opted to use same match function in both
expected and unexpected cases to keep things simple.
One thought that I had was that struct bpf_test might be extended in
the future as follows:
#define MAX_PATTERNS 4
...
struct bpf_test {
...
struct bpf_insn unexpected_insns[MAX_UNEXPECTED_INSNS][MAX_PATTERNS];
...
}
Where each pattern follows logic "seq-A => seq-B", but patterns are
matched independently. So if the goal is to match either "seq-A" or
"seq-B" these should be specified as separate patterns. However, this
seems to be an overkill for the problem at hand.
