From owner-freebsd-hackers@FreeBSD.ORG Mon Nov 9 14:52:13 2009 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 99045106568D for ; Mon, 9 Nov 2009 14:52:13 +0000 (UTC) (envelope-from keramida@freebsd.org) Received: from poseidon.ceid.upatras.gr (poseidon.ceid.upatras.gr [150.140.141.169]) by mx1.freebsd.org (Postfix) with ESMTP id 05D8F8FC0C for ; Mon, 9 Nov 2009 14:52:12 +0000 (UTC) Received: from mail.ceid.upatras.gr (unknown [10.1.0.143]) by poseidon.ceid.upatras.gr (Postfix) with ESMTP id E3672EB47FE; Mon, 9 Nov 2009 16:52:11 +0200 (EET) Received: from localhost (europa.ceid.upatras.gr [127.0.0.1]) by mail.ceid.upatras.gr (Postfix) with ESMTP id C4D2E452A4; Mon, 9 Nov 2009 16:52:11 +0200 (EET) X-Virus-Scanned: amavisd-new at ceid.upatras.gr Received: from mail.ceid.upatras.gr ([127.0.0.1]) by localhost (europa.ceid.upatras.gr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4YMkqdsuC5gk; Mon, 9 Nov 2009 16:52:11 +0200 (EET) Received: from kobe.laptop (ppp-94-64-236-65.home.otenet.gr [94.64.236.65]) by mail.ceid.upatras.gr (Postfix) with ESMTP id 78AA0451B2; Mon, 9 Nov 2009 16:52:11 +0200 (EET) Received: from kobe.laptop (kobe.laptop [127.0.0.1]) by kobe.laptop (8.14.3/8.14.3) with ESMTP id nA9EqATw006008 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 9 Nov 2009 16:52:10 +0200 (EET) (envelope-from keramida@freebsd.org) Received: (from keramida@localhost) by kobe.laptop (8.14.3/8.14.3/Submit) id nA9Eq91N006007; Mon, 9 Nov 2009 16:52:10 +0200 (EET) (envelope-from keramida@freebsd.org) From: Giorgos Keramidas To: Alexander Best References: Date: Mon, 09 Nov 2009 16:52:09 +0200 In-Reply-To: (Alexander Best's message of "Mon, 09 Nov 2009 15:28:29 +0100 (CET)") Message-ID: <87hbt3hht2.fsf@kobe.laptop> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1.50 (berkeley-unix) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: freebsd-hackers@freebsd.org Subject: Re: [patch] burncd: honour for envar SPEED X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Nov 2009 14:52:13 -0000 On Mon, 09 Nov 2009 15:28:29 +0100 (CET), Alexander Best wrote: > Giorgos Keramidas schrieb am 2009-11-09: >> Hi Alexander, > >> The idea seems very good, but since the value of SPEED is user >> supplied data, I would rather see a bit of validation code after >> getenv(). With this version of the patch, burncd would happily >> accept and try to use values that are quite absurd, i.e.: > >> env SPEED=12234567890 burncd ... > >> It may also be sensible to do the translation from "human readable" >> speed values and the multiplication with 177 _after_ the value has >> been parsed from getenv(), so that e.g. one can write: > >> env SPEED=4 burncd > >> and get behavior similar to the current default. > > i don't quite get why the value supplied with the envar has to be > validated. if the user supplies a speed value using the -s switch no > validation (except <= 0) is being performed either. This is probably me being paranoid. I'd prefer *both* places to check the supplied value for invalid values, even if the check is something like "negative numbers are not ok". > also i think there's a speed check in the atapi code. if the speed > requested is > the maximum driver speed it gets set to the maximum > driver speed automatically. If the capping happens automatically we're fine. From a cursory look at the kernel sources this morning, I didn't manage to find a speed-range check in sys/dev/ata. The acd_set_speed() code is a small function: : static int : acd_set_speed(device_t dev, int rdspeed, int wrspeed) : { : int8_t ccb[16] = { ATAPI_SET_SPEED, 0, rdspeed >> 8, rdspeed, : wrspeed >> 8, wrspeed, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; : int error; : : error = ata_atapicmd(dev, ccb, NULL, 0, 0, 30); : if (!error) : acd_get_cap(dev); : return error; : } and that's all. It probably relies on the hardware to cap the speed, but I am not very familiar with the rest of the ATA code to be sure. Your patch is fine, but as a followup commit I'd probably like seeing atoi() go away. AFAICT, it currently allows invalid speed values, defaulting to speed=0 when a user types: burncd -s foobar [options ...] We can fix that later though :)