Date: Mon, 09 Nov 2009 16:52:09 +0200 From: Giorgos Keramidas <keramida@freebsd.org> To: Alexander Best <alexbestms@wwu.de> Cc: freebsd-hackers@freebsd.org Subject: Re: [patch] burncd: honour for envar SPEED Message-ID: <87hbt3hht2.fsf@kobe.laptop> In-Reply-To: <permail-200911091428291e86ffa800007a17-a_best01@message-id.uni-muenster.de> (Alexander Best's message of "Mon, 09 Nov 2009 15:28:29 %2B0100 (CET)") References: <permail-200911091428291e86ffa800007a17-a_best01@message-id.uni-muenster.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 09 Nov 2009 15:28:29 +0100 (CET), Alexander Best <alexbestms@wwu.de> 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 :)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?87hbt3hht2.fsf>