Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751655AbdF1RSM (ORCPT ); Wed, 28 Jun 2017 13:18:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44095 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751501AbdF1RSF (ORCPT ); Wed, 28 Jun 2017 13:18:05 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D81EE78520 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jbenc@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com D81EE78520 Date: Wed, 28 Jun 2017 19:18:01 +0200 From: Jiri Benc To: Matthias Schiffer Cc: davem@davemloft.net, roopa@cumulusnetworks.com, pshelar@ovn.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next 1/2] vxlan: change vxlan_validate() to use netlink_ext_ack for error reporting Message-ID: <20170628191801.1bc8ed67@griffin> In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 28 Jun 2017 17:18:05 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1304 Lines: 37 On Tue, 27 Jun 2017 22:47:57 +0200, Matthias Schiffer wrote: > if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS]))) { > - pr_debug("invalid all zero ethernet address\n"); > + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_ADDRESS], > + "invalid ethernet address"); Could we be more specific here? This is better than nothing but still not as helpful to the user as it could be. What about something like "the provided ethernet address is not unicast"? > - if (mtu < ETH_MIN_MTU || mtu > ETH_MAX_MTU) > + if (mtu < ETH_MIN_MTU || mtu > ETH_MAX_MTU) { > + NL_SET_ERR_MSG_ATTR(extack, tb[IFLA_MTU], > + "invalid MTU"); "MTU must be between 68 and 65535" > - if (id >= VXLAN_N_VID) > + if (id >= VXLAN_N_VID) { > + NL_SET_ERR_MSG_ATTR(extack, data[IFLA_VXLAN_ID], > + "invalid VXLAN ID"); "VXLAN ID must be lower than 16777216" > if (ntohs(p->high) < ntohs(p->low)) { > - pr_debug("port range %u .. %u not valid\n", > - ntohs(p->low), ntohs(p->high)); > + NL_SET_ERR_MSG_ATTR(extack, data[IFLA_VXLAN_PORT_RANGE], > + "port range not valid"); Since you're getting rid of the values output, I'd rather suggest more explicit "the first value of the port range must not be higher than the second value" or so. Shorter wording is welcome :-) Thanks, Jiri