From owner-freebsd-hackers@FreeBSD.ORG Mon Nov 9 19:03:16 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 6D085106566C; Mon, 9 Nov 2009 19:03:16 +0000 (UTC) (envelope-from a_best01@uni-muenster.de) Received: from zivm-exrelay3.uni-muenster.de (ZIVM-EXRELAY3.UNI-MUENSTER.DE [128.176.192.20]) by mx1.freebsd.org (Postfix) with ESMTP id C8C718FC1D; Mon, 9 Nov 2009 19:03:15 +0000 (UTC) X-IronPort-AV: E=Sophos;i="4.44,710,1249250400"; d="scan'208";a="18048238" Received: from zivmaildisp1.uni-muenster.de (HELO ZIVMAILUSER03.UNI-MUENSTER.DE) ([128.176.188.85]) by zivm-relay3.uni-muenster.de with ESMTP; 09 Nov 2009 20:03:14 +0100 Received: by ZIVMAILUSER03.UNI-MUENSTER.DE (Postfix, from userid 149459) id 4B74B1B0751; Mon, 9 Nov 2009 20:03:14 +0100 (CET) Date: Mon, 09 Nov 2009 20:03:13 +0100 (CET) From: Alexander Best Sender: Organization: Westfaelische Wilhelms-Universitaet Muenster To: Giorgos Keramidas Message-ID: In-Reply-To: <87k4xzbmhq.fsf@kobe.laptop> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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 19:03:16 -0000 Giorgos Keramidas schrieb am 2009-11-09: > On Mon, 09 Nov 2009 19:01:43 +0100 (CET), Alexander Best > wrote: > >Giorgos Keramidas schrieb am 2009-11-09: > >> > 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. > >> 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. > Yes, the patch attached below is fine, and IMO it would be ok to > commit > it, minus a couple of tiny details: sorting the BURNCD_SPEED > environment > variable before the current CDROM item in the manpage, and bumping > the > manpage modification date near .Dd to today. > %%% > Index: usr.sbin/burncd/burncd.8 > =================================================================== > --- usr.sbin/burncd/burncd.8 (revision 199064) > +++ usr.sbin/burncd/burncd.8 (working copy) > @@ -164,6 +164,12 @@ > .Fl f > flag. > .El > +.Bl -tag -width ".Ev BURNCD_SPEED" > +.It Ev BURNCD_SPEED > +The write speed to use if one is not specified with the > +.Fl s > +flag. > +.El > .Sh FILES > .Bl -tag -width ".Pa /dev/acd0" > .It Pa /dev/acd0 > Index: usr.sbin/burncd/burncd.c > =================================================================== > --- usr.sbin/burncd/burncd.c (revision 199064) > +++ usr.sbin/burncd/burncd.c (working copy) > @@ -80,11 +80,20 @@ > int dao = 0, eject = 0, fixate = 0, list = 0, multi = 0, > preemp = 0; > int nogap = 0, speed = 4 * 177, test_write = 0, force = 0; > int block_size = 0, block_type = 0, cdopen = 0, dvdrw = 0; > - const char *dev; > + const char *dev, *env_speed; > if ((dev = getenv("CDROM")) == NULL) > dev = "/dev/acd0"; > + if ((env_speed = getenv("BURNCD_SPEED")) != NULL) { > + if (strcasecmp("max", env_speed) == 0) > + speed = CDR_MAX_SPEED; > + else > + speed = atoi(env_speed) * 177; > + if (speed <= 0) > + errx(EX_USAGE, "Invalid speed: %s", > env_speed); > + } > + > while ((ch = getopt(argc, argv, "def:Flmnpqs:tv")) != -1) { > switch (ch) { > case 'd': > %%% ok. thanks a lot. maybe somebody will step up and commit this. i'm not familiar with the man() syntax so i might need some time to add the changes you recommended. also it seems the ENVIREMENT section needs to be aligned differently now that it has more than > 1 entry. cheers. alex