From owner-freebsd-firewire@FreeBSD.ORG Mon Aug 18 19:21:28 2008 Return-Path: Delivered-To: freebsd-firewire@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 19945106567D; Mon, 18 Aug 2008 19:21:28 +0000 (UTC) (envelope-from sbruno@miralink.com) Received: from plato.miralink.com (mail.miralink.com [70.103.185.20]) by mx1.freebsd.org (Postfix) with ESMTP id D767C8FC28; Mon, 18 Aug 2008 19:21:27 +0000 (UTC) (envelope-from sbruno@miralink.com) Received: from localhost (localhost.localdomain [127.0.0.1]) by plato.miralink.com (Postfix) with ESMTP id 524D51A9138; Mon, 18 Aug 2008 12:14:01 -0700 (PDT) X-Virus-Scanned: amavisd-new at X-Spam-Flag: NO X-Spam-Score: -4.399 X-Spam-Level: X-Spam-Status: No, score=-4.399 tagged_above=-10 required=6.6 tests=[ALL_TRUSTED=-1.8, BAYES_00=-2.599] Received: from plato.miralink.com ([127.0.0.1]) by localhost (plato.miralink.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id v1XLgwKPVxKY; Mon, 18 Aug 2008 12:14:00 -0700 (PDT) Received: from [10.0.0.40] (iago.office.miralink.com [10.0.0.40]) by plato.miralink.com (Postfix) with ESMTP id D32BE1A90EA; Mon, 18 Aug 2008 12:14:00 -0700 (PDT) Message-ID: <48A9CBB5.6030402@miralink.com> Date: Mon, 18 Aug 2008 12:21:25 -0700 From: Sean Bruno User-Agent: Thunderbird 2.0.0.16 (X11/20080723) MIME-Version: 1.0 To: Dieter References: <200808181913.TAA21449@sopwith.solgatos.com> In-Reply-To: <200808181913.TAA21449@sopwith.solgatos.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Scott Long , freebsd-firewire@freebsd.org Subject: Re: fwcontrol update X-BeenThere: freebsd-firewire@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Firewire support in FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Aug 2008 19:21:28 -0000 Dieter wrote: > case 'b': > > if (priority_budget < 0 || priority_budget > INT32_MAX) > errx(EX_USAGE, "%s: invalid number: %s", __func__, optarg); > > case 'f': > > if ( (adjust_gap_count < 0) || (adjust_gap_count > INT32_MAX) ) > err(EX_USAGE, "%s:adjust_gap_count out of range", __func__); > > > I think "out of range" is better than "invalid number". > -5 is a valid number. > current_board = strtol(optarg, NULL, 0); > Just a minor nit, feel free to ignore this one. :-) > > ================================================================================ > > Agreed. It's a nit, but I'll change it. >> case 'c': >> crom_string = malloc(strlen(optarg)+1); >> if (crom_string == NULL) >> err(EX_SOFTWARE, "%s:crom_string malloc", __func__); >> if ( (strtol(crom_string, NULL, 0) < 0) || strtol(crom_string, NULL, 0) > MAX_BOARDS) >> err(EX_USAGE, "%s:Invalid value for node", __func__); >> strcpy(crom_string, optarg); >> > > Strtol() reads freshly malloc-ed memory before anything has been put there. > > Perhaps: > > case 'c': > { > long node_num; > node_num = strtol(optarg, NULL, 0); > if ( (node_num < 0) || (node_num > MAX_BOARDS) ) > err(EX_USAGE, "%s:node out of range", __func__); > crom_string = malloc(strlen(optarg)+1); > if (crom_string == NULL) > err(EX_SOFTWARE, "%s:crom_string malloc", __func__); > strcpy(crom_string, optarg); > > ... > > } > case 'd': > > > ================================================================================ > > This is a big ol' whoops on my part. The whole "malloc" and copy thing just seems pointless and needs to be consolidated into just a "strtol()" instead. > case 'u': > > current_board = strtol(optarg, NULL, 0); > > Does this need a range check? > > ================================================================================ > Probably should be a range check( current_board < 0 and current_board > MAX_BOARDS). -- Sean Bruno MiraLink Corporation 6015 NE 80th Ave, Ste 100 Portland, OR 97218 Phone 503-621-5143 Fax 503-621-5199 MSN: sbruno@miralink.com Google: seanwbruno@gmail.com Yahoo: sean_bruno@yahoo.com