Date: Mon, 09 Nov 2009 19:01:43 +0100 (CET) From: Alexander Best <alexbestms@wwu.de> To: Giorgos Keramidas <keramida@freebsd.org> Cc: freebsd-hackers@freebsd.org Subject: Re: [patch] burncd: honour for envar SPEED Message-ID: <permail-20091109180143f0889e84000046bf-a_best01@message-id.uni-muenster.de> In-Reply-To: <87hbt3hht2.fsf@kobe.laptop>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a MIME encoded multipart message.
--+permail-20091109180143f0889e84000046bf-a_best01+
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Giorgos Keramidas schrieb am 2009-11-09:
> 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 :)
ok. so do you think this patch is sufficient then? once committed i'll see if
i can add some extra validation to the envar as well as the -s switch and will
also have a look at the validation the ATA code is doing atm.
alex
--+permail-20091109180143f0889e84000046bf-a_best01+
Content-Type: text/plain
Content-Transfer-Encoding: Base64
Content-Disposition: attachment; filename="burncdspeedpatch.txt"
SW5kZXg6IHVzci5zYmluL2J1cm5jZC9idXJuY2QuOAo9PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSB1c3Iuc2Jpbi9i
dXJuY2QvYnVybmNkLjgJKHJldmlzaW9uIDE5OTA2NCkKKysrIHVzci5zYmluL2J1cm5jZC9idXJu
Y2QuOAkod29ya2luZyBjb3B5KQpAQCAtMTY0LDYgKzE2NCwxMiBAQAogLkZsIGYKIGZsYWcuCiAu
RWwKKy5CbCAtdGFnIC13aWR0aCAiLkV2IEJVUk5DRF9TUEVFRCIKKy5JdCBFdiBCVVJOQ0RfU1BF
RUQKK1RoZSB3cml0ZSBzcGVlZCB0byB1c2UgaWYgb25lIGlzIG5vdCBzcGVjaWZpZWQgd2l0aCB0
aGUKKy5GbCBzCitmbGFnLgorLkVsCiAuU2ggRklMRVMKIC5CbCAtdGFnIC13aWR0aCAiLlBhIC9k
ZXYvYWNkMCIKIC5JdCBQYSAvZGV2L2FjZDAKSW5kZXg6IHVzci5zYmluL2J1cm5jZC9idXJuY2Qu
Ywo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09Ci0tLSB1c3Iuc2Jpbi9idXJuY2QvYnVybmNkLmMJKHJldmlzaW9uIDE5OTA2
NCkKKysrIHVzci5zYmluL2J1cm5jZC9idXJuY2QuYwkod29ya2luZyBjb3B5KQpAQCAtODAsMTEg
KzgwLDIwIEBACiAJaW50IGRhbyA9IDAsIGVqZWN0ID0gMCwgZml4YXRlID0gMCwgbGlzdCA9IDAs
IG11bHRpID0gMCwgcHJlZW1wID0gMDsKIAlpbnQgbm9nYXAgPSAwLCBzcGVlZCA9IDQgKiAxNzcs
IHRlc3Rfd3JpdGUgPSAwLCBmb3JjZSA9IDA7CiAJaW50IGJsb2NrX3NpemUgPSAwLCBibG9ja190
eXBlID0gMCwgY2RvcGVuID0gMCwgZHZkcncgPSAwOwotCWNvbnN0IGNoYXIgKmRldjsKKwljb25z
dCBjaGFyICpkZXYsICplbnZfc3BlZWQ7CiAKIAlpZiAoKGRldiA9IGdldGVudigiQ0RST00iKSkg
PT0gTlVMTCkKIAkJZGV2ID0gIi9kZXYvYWNkMCI7CiAKKwlpZiAoKGVudl9zcGVlZCA9IGdldGVu
digiQlVSTkNEX1NQRUVEIikpICE9IE5VTEwpIHsKKwkJaWYgKHN0cmNhc2VjbXAoIm1heCIsIGVu
dl9zcGVlZCkgPT0gMCkKKwkJCXNwZWVkID0gQ0RSX01BWF9TUEVFRDsKKwkJZWxzZQorCQkJc3Bl
ZWQgPSBhdG9pKGVudl9zcGVlZCkgKiAxNzc7CisJCWlmIChzcGVlZCA8PSAwKQorCQkJZXJyeChF
WF9VU0FHRSwgIkludmFsaWQgc3BlZWQ6ICVzIiwgZW52X3NwZWVkKTsKKwl9CisKIAl3aGlsZSAo
KGNoID0gZ2V0b3B0KGFyZ2MsIGFyZ3YsICJkZWY6RmxtbnBxczp0diIpKSAhPSAtMSkgewogCQlz
d2l0Y2ggKGNoKSB7CiAJCWNhc2UgJ2QnOgo=
--+permail-20091109180143f0889e84000046bf-a_best01+--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?permail-20091109180143f0889e84000046bf-a_best01>