lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 21 Oct 2015 12:20:05 +0200 From: Ingo Molnar <mingo@...nel.org> To: He Kuang <hekuang@...wei.com> Cc: ast@...nel.org, davem@...emloft.net, daniel@...earbox.net, rostedt@...dmis.org, xiakaixu@...wei.com, ast@...mgrid.com, wangnan0@...wei.com, linux-kernel@...r.kernel.org Subject: Re: [RFC PATCH] bpf: Add new bpf map type for timer * He Kuang <hekuang@...wei.com> wrote: > ping and add ast@...mgrid.com, what's your opinion on this? Firstly, two days isn't nearly enough for a 'review timeout', secondly, have you seen the kbuild test reports? Thirdly, I suspect others will do a deeper review, but even stylistically the patch is a bit weird, for example these kinds of unstructured struct initializers are annoying: > > struct bpf_map_def SEC("maps") timer_map = { > > .type = BPF_MAP_TYPE_TIMER_ARRAY, > > .key_size = sizeof(int), > > .value_size = sizeof(unsigned long long), > > .max_entries = 4, > > }; > > .map_alloc = fd_array_map_alloc, > > .map_free = fd_array_map_free, > > .map_get_next_key = array_map_get_next_key, > >- .map_lookup_elem = fd_array_map_lookup_elem, > >+ .map_lookup_elem = empty_array_map_lookup_elem, > > .map_update_elem = fd_array_map_update_elem, > > .map_delete_elem = fd_array_map_delete_elem, > > .map_fd_get_ptr = prog_fd_array_get_ptr, > >@@ -312,7 +318,7 @@ static const struct bpf_map_ops perf_event_array_ops = { > > .map_alloc = fd_array_map_alloc, > > .map_free = perf_event_array_map_free, > > .map_get_next_key = array_map_get_next_key, > >- .map_lookup_elem = fd_array_map_lookup_elem, > >+ .map_lookup_elem = empty_array_map_lookup_elem, > > .map_update_elem = fd_array_map_update_elem, > > .map_delete_elem = fd_array_map_delete_elem, > > .map_fd_get_ptr = perf_event_fd_array_get_ptr, > >+static const struct bpf_map_ops timer_array_ops = { > >+ .map_alloc = timer_array_map_alloc, > >+ .map_free = timer_array_map_free, > >+ .map_get_next_key = array_map_get_next_key, > >+ .map_lookup_elem = empty_array_map_lookup_elem, > >+ .map_update_elem = timer_array_map_update_elem, > >+ .map_delete_elem = timer_array_map_delete_elem, > >+}; > >+ > >+static struct bpf_map_type_list timer_array_type __read_mostly = { > >+ .ops = &timer_array_ops, > >+ .type = BPF_MAP_TYPE_TIMER_ARRAY, > >+}; Please align initializations vertically, so the second column becomes readable, patterns in them become easy to see and individual entries become easier to compare. See for example kernel/sched/core.c: struct cgroup_subsys cpu_cgrp_subsys = { .css_alloc = cpu_cgroup_css_alloc, .css_free = cpu_cgroup_css_free, .css_online = cpu_cgroup_css_online, .css_offline = cpu_cgroup_css_offline, .fork = cpu_cgroup_fork, .can_attach = cpu_cgroup_can_attach, .attach = cpu_cgroup_attach, .exit = cpu_cgroup_exit, .legacy_cftypes = cpu_files, .early_init = 1, }; That's a _lot_ more readable than: struct cgroup_subsys cpu_cgrp_subsys = { .css_alloc = cpu_cgroup_css_alloc, .css_free = cpu_cgroup_css_free, .css_online = cpu_cgroup_css_online, .css_offline = cpu_cgroup_css_offline, .fork = cpu_cgroup_fork, .can_attach = cpu_cgroup_attach, .attach = cpu_cgroup_attach, .exit = cpu_cgroup_exit, .legacy_cftypes = cpu_files, .early_init = 1, }; right? For example I've hidden a small initialization bug into the second variant, how much time does it take for you to notice it? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists