Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 1 Aug 2022 10:00:59 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Phillip Potter <phil@philpotter.co.uk>
Cc:     gregkh@linuxfoundation.org, Larry.Finger@lwfinger.net,
        paskripkin@gmail.com, martin@kaiser.cx, straube.linux@gmail.com,
        fmdefrancesco@gmail.com, abdun.nihaal@gmail.com,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] staging: r8188eu: convert rtw_set_802_11_add_wep
 error code semantics
Message-ID: <20220801070059.GA3460@kadam>
References: <20220728231150.972-1-phil@philpotter.co.uk>
 <20220728231150.972-3-phil@philpotter.co.uk>
 <20220729064803.GT2338@kadam>
 <YuV452xuR1S0WyJi@OEMBP14.local>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YuV452xuR1S0WyJi@OEMBP14.local>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?XBGWuZXK3s2Zdl1DRWNJhtN02YvgCwiEhIHLXG90nmprO2xKjqDVHsSHSnSu?=
 =?us-ascii?Q?v1VEdtdy5QWqJWZ+din8pU4aHzlG2ipwSDV4Ux+9tTwGsSYxyjRs0rFE9zjQ?=
 =?us-ascii?Q?UNB4bKNqCQRdFrcYu+KQ1PZWhfY9gg2OgyHUDAjQm/Rsmz3uxRUKLtRqsYbT?=
 =?us-ascii?Q?TbWCbyCyjNXbNU/wlZdnh5yM2C25uX3OcMxYnhgcJBJaeAWS45k10dujaQhh?=
 =?us-ascii?Q?ojBnUB9VsH2XpGJWNcK5GpU/QP6jySRD8+JntMwwNZPEbMfUDzN/wK8WIpg9?=
 =?us-ascii?Q?2YcIpFvFJN1xPpumsRlRoM/XAs3hwYo/aN93+3LlQuDdTk1ruAHmIt4KJLCg?=
 =?us-ascii?Q?rO8SV5OZ4gJwMxUQ+idyZNO84aL0kQGMHYBKrnxnSagZN3+1SqhYeMuOdkCC?=
 =?us-ascii?Q?Ox0fxvSWTdthY5QDycUyrXA63B0EsM0w/a0uDHWU2r6VHr09m9euMugPdx0+?=
 =?us-ascii?Q?S5GCcRvmAS0Ejj6ytoLBwkNKfTG9lyicfwEsbzWIIm+A474IPehrxMtaHMa7?=
 =?us-ascii?Q?E2TKCDXOn94UGL3hf6bQwQmU8XgdxDmszsjbD23Vg4k+gzrwcumpufwTJQhe?=
 =?us-ascii?Q?PKzf1ZtXbkgx2RB7+D1JDvXiOJem+U8Gu4xNTc9TeOa6iaaH5Sa178g/zLcT?=
 =?us-ascii?Q?NjK9CVG6TeInCpQBZgY2IUDbdQozmTvr4IOSWmcLCXuDjNyKsO/jvOLL4USu?=
 =?us-ascii?Q?9DS9+hM3HXF6XIWwmKgxKldguwfLiYbEEXYFYOdbyiHypmEj7I8Y/OmV5uqV?=
 =?us-ascii?Q?NDivMjZPVKwUq9GYCXvbDm2iF2VKWCrV0fVe60B/b8Jtx5ISAuefUKiLWIuc?=
 =?us-ascii?Q?fVtCijxefIPlx34ro85jR0LYXEGgGMkXFcIcOrJc2QpShzX0Rz6aMRBm6zZn?=
 =?us-ascii?Q?9BlpbIuNiq672R8I/nuiyZ+xoeNPeSp8l6hUPLcBTjIeoQOmbw5hENeTgV5w?=
 =?us-ascii?Q?/FCYz8eVy/Eec0/oZ4JA3tHXAuZ8npxoSnB3Ff3veIm8ghzDf0QNX3RFsGft?=
 =?us-ascii?Q?6/73Nd4G2g4lTEgaV9AQcIdDn6NmyGy/wW20iaB5bUJXPHVqR3SrR2pmKdKu?=
 =?us-ascii?Q?N26JyuWZt4sgy/0E6EktisIJ7fJkw3lS8uZ395rO43M+G6plUxo2Dk74G/s3?=
 =?us-ascii?Q?Sw8LtfKEwcnzADw5pDeZ6eiv/eZHdvtyvwsdNzb5bq3qZqzWi533H0vTjj7I?=
 =?us-ascii?Q?Ti1rPybR8Ini+iGSqOCgMV5DIf/F2StD5VtrXkwNkS0VpD5XWPk3MHbXNoC4?=
 =?us-ascii?Q?zXgGX2Hga7tnmis28cA9LM9UVeA2qof3/t764Q7K7EXKMr9IP2mOH5mXyWPb?=
 =?us-ascii?Q?Z6rdGlEpmGr5IE+i3QqSiyUNgw+ufZQCiV9fi0DaA7qLJy3UtmSX3fjSHrsF?=
 =?us-ascii?Q?ad3M9behg2D3dMSwy/3ko1uCDRPAPr15iDI5bSedJQQEeuenCa6rH2035xiU?=
 =?us-ascii?Q?C/BAErHxV4hcAGgx1CIEUGSR7vWcCUS7e6OJMF1+UJPLR2GGwBp+OKcR4y4k?=
 =?us-ascii?Q?Kf5B4leQzRS1An4uI6VsMFFORDChGd24mqUlHaCSvRPfcaKCEeUlpCUQdg7B?=
 =?us-ascii?Q?ZeWXjjvWHWvQ3x8TMebSF5PUoq5ApKn27ydhYdJZ?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a933e809-ee9d-4089-e69d-08da738b9d95
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Aug 2022 07:01:11.1122
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yYxsWc1mesGlmv8TCwr/9MAUGJDcJtHY0nEMLcT1jnlAUxXkiqGLCGzM4VO9bWPlCDVxaR1lp3wsvhhdV6rQQK27gT4w//XNqxIyp3OSwDQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5200
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-01_02,2022-07-28_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 suspectscore=0
 mlxlogscore=999 bulkscore=0 mlxscore=0 phishscore=0 spamscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2206140000 definitions=main-2208010033
X-Proofpoint-ORIG-GUID: FMTGsynb4UZOEq6eWJ5HWYScMq6i7Tfy
X-Proofpoint-GUID: FMTGsynb4UZOEq6eWJ5HWYScMq6i7Tfy
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sat, Jul 30, 2022 at 07:36:57PM +0100, Phillip Potter wrote:
> On Fri, Jul 29, 2022 at 09:48:03AM +0300, Dan Carpenter wrote:
> > On Fri, Jul 29, 2022 at 12:11:50AM +0100, Phillip Potter wrote:
> > > -u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> > > +int rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> > >  {
> > >  	int		keyid, res;
> > >  	struct security_priv *psecuritypriv = &padapter->securitypriv;
> > > -	u8		ret = _SUCCESS;
> > > +	int		ret = 0;
> > >  
> > >  	keyid = wep->KeyIndex & 0x3fffffff;
> > >  
> > >  	if (keyid >= 4) {
> > > -		ret = false;
> > > +		ret = -EOPNOTSUPP;
> > >  		goto exit;
> > >  	}
> > >  
> > > @@ -424,7 +424,7 @@ u8 rtw_set_802_11_add_wep(struct adapter *padapter, struct ndis_802_11_wep *wep)
> > >  	res = rtw_set_key(padapter, psecuritypriv, keyid, 1);
> > >  
> > >  	if (res == _FAIL)
> > > -		ret = false;
> > > +		ret = -ENOMEM;
> > >  exit:
> > >  
> > >  	return ret;
> > 
> > No, this isn't right.  This now returns 1 on success and negative
> > error codes on error.
> > 
> > There are a couple anti-patterns here:
> > 
> > 1) Do nothing gotos
> > 2) Mixing error paths and success paths.
> > 
> > If you avoid mixing error paths and success paths then you get a pattern
> > called: "Solid return zero."  This is where the end of the function has
> > a very chunky "return 0;" to mark that it is successful.  You want that.
> > Some people do a "if (ret == 0) return ret;".  Nope.  "return ret;" is
> > not chunky.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Hi Dan,
> 
> Thank you for the review firstly, much appreciated.
> 
> I'm happy of course to rewrite this to address any concerns, but
> I was hoping I could clarify what you've said though? Apologies if I've
> missed it, but how is this function now returning 1 on success? It sets
> ret to 0 (success) at the start and then sets it to one of two negative
> error codes depending on what happens. Am I missing something here?
> (Perfectly possible that I am).

You're right.  I misread "res" as "ret".  It's another anti-pattern to
have two "ret" variables.  The code is fine but so ugly for no reason.

regards,
dan carpenter