Date: Mon, 18 Aug 2008 12:21:25 -0700 From: Sean Bruno <sbruno@miralink.com> To: Dieter <freebsd@sopwith.solgatos.com> Cc: Scott Long <scottl@samsco.org>, freebsd-firewire@freebsd.org Subject: Re: fwcontrol update Message-ID: <48A9CBB5.6030402@miralink.com> In-Reply-To: <200808181913.TAA21449@sopwith.solgatos.com> References: <200808181913.TAA21449@sopwith.solgatos.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?48A9CBB5.6030402>