Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jonathan Toppins <jtoppins@redhat.com>
To: Leon Romanovsky <leon@kernel.org>
Cc: netdev@vger.kernel.org, Jay Vosburgh <j.vosburgh@gmail.com>,
	Veaceslav Falico <vfalico@gmail.com>,
	Andy Gospodarek <andy@greyhouse.net>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 2/2] bonding: combine netlink and console error messages
Date: Sun, 8 Aug 2021 21:42:46 -0400	[thread overview]
Message-ID: <14b506c3-7e8d-f313-b585-4e7ff1a542cf@redhat.com> (raw)
In-Reply-To: <YQ+vDtXPV5DHqruU@unreal>

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@redhat.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


  reply	other threads:[~2021-08-09  1:42 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-07  3:30 [PATCH net-next 0/2] bonding: cleanup header file and error msgs Jonathan Toppins
2021-08-07  3:30 ` [PATCH net-next 1/2] bonding: remove extraneous definitions from bonding.h Jonathan Toppins
2021-08-07  3:30 ` [PATCH net-next 2/2] bonding: combine netlink and console error messages Jonathan Toppins
2021-08-07  3:52   ` Joe Perches
2021-08-07 21:54     ` Jonathan Toppins
2021-08-08 10:02       ` Joe Perches
2021-08-09  2:07         ` Jonathan Toppins
2021-08-09  5:05           ` Joe Perches
2021-08-09 17:17             ` Jonathan Toppins
2021-08-08 10:16   ` Leon Romanovsky
2021-08-09  1:42     ` Jonathan Toppins [this message]
2021-08-09  6:03       ` Leon Romanovsky
2021-08-10  6:40   ` [PATCH net-next v2 " Jonathan Toppins
2021-08-10  6:47     ` Leon Romanovsky
2021-08-11  2:53 ` [PATCH net-next v2 0/2] bonding: cleanup header file and error msgs Jonathan Toppins
2021-08-11  2:53   ` [PATCH net-next v2 1/2] bonding: remove extraneous definitions from bonding.h Jonathan Toppins
2021-08-11  2:53   ` [PATCH net-next v2 2/2] bonding: combine netlink and console error messages Jonathan Toppins
2021-08-11  3:27     ` Joe Perches
2021-08-11 12:49       ` Jakub Kicinski
2021-08-11 13:23         ` Joe Perches
2021-08-11 14:31           ` Jonathan Toppins
2021-08-11 22:10   ` [PATCH net-next v2 0/2] bonding: cleanup header file and error msgs patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=14b506c3-7e8d-f313-b585-4e7ff1a542cf@redhat.com \
    --to=jtoppins@redhat.com \
    --cc=andy@greyhouse.net \
    --cc=davem@davemloft.net \
    --cc=j.vosburgh@gmail.com \
    --cc=kuba@kernel.org \
    --cc=leon@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vfalico@gmail.com \
    --subject='Re: [PATCH net-next 2/2] bonding: combine netlink and console error messages' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).