From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752420AbcHHUSg (ORCPT ); Mon, 8 Aug 2016 16:18:36 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:34968 "EHLO mx3-phx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751987AbcHHUSb (ORCPT ); Mon, 8 Aug 2016 16:18:31 -0400 Date: Mon, 8 Aug 2016 16:17:26 -0400 (EDT) From: Laurence Oberman To: David Binderman Cc: jejb@linux.vnet.ibm.com, martin petersen , linux-scsi@vger.kernel.org, Linux Kernel Mailing List , dcb314@hotmail.com Message-ID: <1029353111.181631.1470687446200.JavaMail.zimbra@redhat.com> In-Reply-To: References: Subject: Re: linux-4.8-rc1/drivers/scsi/sd.c:317: pointless test ? MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [10.18.49.4] X-Mailer: Zimbra 8.0.6_GA_5922 (ZimbraWebClient - FF38 (Linux)/8.0.6_GA_5922) Thread-Topic: linux-4.8-rc1/drivers/scsi/sd.c:317: pointless test ? Thread-Index: ijbjpxmHqOw6BuKT6+bVS+8+x/qIsw== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- Original Message ----- > From: "David Binderman" > To: jejb@linux.vnet.ibm.com, "martin petersen" , linux-scsi@vger.kernel.org, "Linux > Kernel Mailing List" , dcb314@hotmail.com > Sent: Monday, August 8, 2016 9:46:53 AM > Subject: linux-4.8-rc1/drivers/scsi/sd.c:317: pointless test ? > > Hello there, > > linux-4.8-rc1/drivers/scsi/sd.c:317]: (style) Unsigned variable 'val' > can't be negative so it is unnecessary to test it. > > Source code is > > if (val >= 0 && val <= SD_DIF_TYPE3_PROTECTION) > > but > > unsigned int val; > > Suggest new code > > if (val <= SD_DIF_TYPE3_PROTECTION) > > Regards > > David Binderman > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Hello Declaring val as unsigned does not prevent it from being assigned a negative integer. This means there is a possibility it could be negative. However looking at the code it seems that we are converting char *buf (string) to an int to set val here. Also the kernel code shows we cannot have a - sign anyway so I expect your suggestion should be fine to change this. static ssize_t protection_type_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct scsi_disk *sdkp = to_scsi_disk(dev); unsigned int val; int err; if (!capable(CAP_SYS_ADMIN)) return -EACCES; err = kstrtouint(buf, 10, &val); ***** if (err) return err; if (val >= 0 && val <= SD_DIF_TYPE3_PROTECTION) sdkp->protection_type = val; Reviewed-by Laurence Oberman return count; } /** * kstrtouint - convert a string to an unsigned int * @s: The start of the string. The string must be null-terminated, and may also * include a single newline before its terminating null. The first character * may also be a plus sign, but not a minus sign. * @base: The number base to use. The maximum supported base is 16. If base is * given as 0, then the base of the string is automatically detected with the * conventional semantics - If it begins with 0x the number will be parsed as a * hexadecimal (case insensitive), if it otherwise begins with 0, it will be * parsed as an octal number. Otherwise it will be parsed as a decimal. * @res: Where to write the result of the conversion on success. * * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error. * Used as a replacement for the obsolete simple_strtoull. Return code must * be checked. */ int kstrtouint(const char *s, unsigned int base, unsigned int *res) { unsigned long long tmp; int rv; rv = kstrtoull(s, base, &tmp); if (rv < 0) return rv; if (tmp != (unsigned long long)(unsigned int)tmp) return -ERANGE; *res = tmp; return 0; }