netdev - Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 Dec 2017 00:00:21 +0100
From:   Philippe Ombredanne <pombredanne@...b.com>
To:     Roman Gushchin <guro@...com>
Cc:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        netdev@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        kernel-team@...com, ast@...nel.org, daniel@...earbox.net,
        kafai@...com, Quentin Monnet <quentin.monnet@...ronome.com>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

Roman,

On Thu, Dec 7, 2017 at 11:23 PM, Jakub Kicinski
<jakub.kicinski@...ronome.com> wrote:
> On Thu, 7 Dec 2017 18:39:09 +0000, Roman Gushchin wrote:
>> This patch adds basic cgroup bpf operations to bpftool:
>> cgroup list, attach and detach commands.
>>
>> Usage is described in the corresponding man pages,
>> and examples are provided.
>>
>> Syntax:
>> $ bpftool cgroup list CGROUP
>> $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]
>> $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG
>>
>> Signed-off-by: Roman Gushchin <guro@...com>
>
> Looks good, a few very minor nits/questions below.
>
>> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
>> new file mode 100644
>> index 000000000000..88d67f74313f
>> --- /dev/null
>> +++ b/tools/bpf/bpftool/cgroup.c
>> @@ -0,0 +1,305 @@
>> +/*
>> + * Copyright (C) 2017 Facebook
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + *
>> + * Author: Roman Gushchin <guro@...com>
>> + */

Have you considered using the new SPDX ids instead?
e.g. may be something like:
// SPDX-License-Identifier: GPL-2.0+
// Copyright (C) 2017 Facebook
// Author: Roman Gushchin <guro@...com>

Don't you love it with less boilerplate and a better code/comments ratio?
BTW the comment style may surprise you here: this is a suggestion, but
not just. Check the posts from Linus on this topic and Thomas's doc
patches for the rationale.

Thank you for your kind consideration!

-- 
Cordially
Philippe Ombredanne