From owner-freebsd-hackers@FreeBSD.ORG Mon Nov 9 18:01:52 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 8DA79106566C; Mon, 9 Nov 2009 18:01:52 +0000 (UTC) (envelope-from a_best01@uni-muenster.de) Received: from zivm-exrelay1.uni-muenster.de (ZIVM-EXRELAY1.UNI-MUENSTER.DE [128.176.192.14]) by mx1.freebsd.org (Postfix) with ESMTP id D68C68FC0C; Mon, 9 Nov 2009 18:01:51 +0000 (UTC) X-IronPort-AV: E=Sophos;i="4.44,710,1249250400"; d="txt'?scan'208";a="287825002" Received: from zivmaildisp1.uni-muenster.de (HELO ZIVMAILUSER01.UNI-MUENSTER.DE) ([128.176.188.85]) by zivm-relay1.uni-muenster.de with ESMTP; 09 Nov 2009 19:01:50 +0100 Received: by ZIVMAILUSER01.UNI-MUENSTER.DE (Postfix, from userid 149459) id D80FA1B0767; Mon, 9 Nov 2009 19:01:49 +0100 (CET) Date: Mon, 09 Nov 2009 19:01:43 +0100 (CET) From: Alexander Best Sender: Organization: Westfaelische Wilhelms-Universitaet Muenster To: Giorgos Keramidas Message-ID: In-Reply-To: <87hbt3hht2.fsf@kobe.laptop> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary=+permail-20091109180143f0889e84000046bf-a_best01+ 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 18:01:52 -0000 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 > 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+--