Date: Mon, 09 Nov 2009 20:03:13 +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-200911091903131e86ffa800005c56-a_best01@message-id.uni-muenster.de> In-Reply-To: <87k4xzbmhq.fsf@kobe.laptop>
next in thread | previous in thread | raw e-mail | index | archive | help
Giorgos Keramidas schrieb am 2009-11-09:
> On Mon, 09 Nov 2009 19:01:43 +0100 (CET), Alexander Best
> <alexbestms@wwu.de> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?permail-200911091903131e86ffa800005c56-a_best01>