netdev - Re: [PATCH net-next 2/2] bonding: combine netlink and console error messages

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]

Message-ID: <14b506c3-7e8d-f313-b585-4e7ff1a542cf@redhat.com>
Date:   Sun, 8 Aug 2021 21:42:46 -0400
From:   Jonathan Toppins <jtoppins@...hat.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     netdev@...r.kernel.org, Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] bonding: combine netlink and console error
 messages

On 8/8/21 6:16 AM, Leon Romanovsky wrote:
> On Fri, Aug 06, 2021 at 11:30:55PM -0400, Jonathan Toppins wrote:
>> There seems to be no reason to have different error messages between
>> netlink and printk. It also cleans up the function slightly.
>>
>> Signed-off-by: Jonathan Toppins <jtoppins@...hat.com>
>> ---
>>   drivers/net/bonding/bond_main.c | 45 ++++++++++++++++++---------------
>>   1 file changed, 25 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 3ba5f4871162..46b95175690b 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1712,6 +1712,16 @@ void bond_lower_state_changed(struct slave *slave)
>>   	netdev_lower_state_changed(slave->dev, &info);
>>   }
>>   
>> +#define BOND_NL_ERR(bond_dev, extack, errmsg) do {		\
>> +	NL_SET_ERR_MSG(extack, errmsg);				\
>> +	netdev_err(bond_dev, "Error: " errmsg "\n");		\
>> +} while (0)
>> +
>> +#define SLAVE_NL_ERR(bond_dev, slave_dev, extack, errmsg) do {	\
>> +	NL_SET_ERR_MSG(extack, errmsg);				\
>> +	slave_err(bond_dev, slave_dev, "Error: " errmsg "\n");	\
>> +} while (0)
> 
> I don't think that both extack messages and dmesg prints are needed.
> 
> They both will be caused by the same source, and both will be seen by
> the caller, but duplicated.
> 
> IMHO, errors that came from the netlink, should be printed with NL_SET_ERR_MSG(),
> other errors should use netdev_err/slave_err prints.
> 

bond_enslave can be called from two places sysfs and netlink so 
reporting both a console message and netlink message makes sense to me. 
So I have to disagree in this case. I am simply making the two paths 
report the same error in the function so that when using sysfs the same 
information is reported. In the netlink case the information will be 
reported twice, once an an error response over netlink and once via printk.

-Jon