[patch] burncd: honour for envar SPEED

[Alexander Best]

Giorgos Keramidas schrieb am 2009-11-09:
> On Mon, 09 Nov 2009 15:28:29 +0100 (CET), Alexander Best
> <alexbestms at 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
-------------- next part --------------
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':