On 01/15/2018 12:52 AM, Jakub Kicinski wrote:
> On Sun, Jan 14, 2018 at 3:37 PM, Daniel Borkmann <dan...@iogearbox.net> wrote:
>> Hi Jakub,
>>
>> Series looks fine, just stumbled over one small thing here below.
>>
>> On 01/12/2018 05:29 AM, Jakub Kicinski wrote:
>> [...]
>>> +bool bpf_offload_dev_match(struct bpf_prog *prog, struct bpf_map *map)
>>> +{
>>> + struct bpf_offloaded_map *offmap;
>>> + struct bpf_prog_offload *offload;
>>> + bool ret;
>>> +
>>> + if (!!bpf_prog_is_dev_bound(prog->aux) != !!bpf_map_is_dev_bound(map))
>>> + return false;
>>> + if (!bpf_prog_is_dev_bound(prog->aux))
>>> + return true;
>>
>> Should this not say 'false' if the prog has no offload_requested ...
>>
>>> + down_read(&bpf_devs_lock);
>>> + offload = prog->aux->offload;
>>> + offmap = map_to_offmap(map);
>>> +
>>> + ret = offload && offload->netdev == offmap->netdev;
>>
>> ... meaning we return true from bpf_offload_dev_match() only in the
>> case when netdevs match?
>
> IOW return false when both program and map are not offloaded? I was
> going for "are those two compatible" kind of logic.
>
> But I'll change, the only user of this function is the verifier
> compatibility check and that already handles the "neither is
> offloaded" case.
Yeah, agree, it's redundant, but not a bug. I'm fine if you roll this into
your follow-ups, since netdevsim and test cases are still to come anyway.
