|
[patch] burncd: honour for envar SPEED
any thoughts on these small changes to burncd?
alex
Index: usr.sbin/burncd/burncd.c
===================================================================
--- usr.sbin/burncd/burncd.c (revision 199064)
+++ usr.sbin/burncd/burncd.c (working copy)
@@ -78,13 +78,16 @@
{
int arg, addr, ch, fd;
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 nogap = 0, speed = 0, test_write = 0, force = 0;
int block_size = 0, block_type = 0, cdopen = 0, dvdrw = 0;
const char *dev;
if ((dev = getenv("CDROM")) == NULL)
dev = "/dev/acd0";
+ if ((speed = getenv("SPEED")) == NULL)
+ speed = 4 * 177;
+
while ((ch = getopt(argc, argv, "def:Flmnpqs:tv")) != -1) {
switch (ch) {
case 'd':
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 SPEED"
+.It Ev 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
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
Alexander Best escribió:
> any thoughts on these small changes to burncd?
>
> - int nogap = 0, speed = 4 * 177, test_write = 0, force = 0;
> + int nogap = 0, speed = 0, test_write = 0, force = 0;
> int block_size = 0, block_type = 0, cdopen = 0, dvdrw = 0;
> const char *dev;
>
> if ((dev = getenv("CDROM")) == NULL)
> dev = "/dev/acd0";
>
> + if ((speed = getenv("SPEED")) == NULL)
> + speed = 4 * 177;
> +
It seems incorrect. The speed variable is of type int, while getenv
returns char *. You should first assign getenv("SPEED") to a char *
variable and if it isn't NULL then you should convert it to int or fall
back to the default value otherwise.
--
Gabor Kovesdan
FreeBSD Volunteer
EMAIL: gabor@... .:|:. gabor@...
WEB: http://people.FreeBSD.org/~gabor .:|:. http://kovesdan.org_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
Gabor Kovesdan escribió:
> Alexander Best escribió:
>> any thoughts on these small changes to burncd?
>> - int nogap = 0, speed = 4 * 177, test_write = 0, force = 0;
>> + int nogap = 0, speed = 0, test_write = 0, force = 0;
>> int block_size = 0, block_type = 0, cdopen = 0, dvdrw = 0;
>> const char *dev;
>>
>> if ((dev = getenv("CDROM")) == NULL)
>> dev = "/dev/acd0";
>>
>> + if ((speed = getenv("SPEED")) == NULL)
>> + speed = 4 * 177;
>> +
> It seems incorrect. The speed variable is of type int, while getenv
> returns char *. You should first assign getenv("SPEED") to a char *
> variable and if it isn't NULL then you should convert it to int or
> fall back to the default value otherwise.
And one more thing. Personally, I think that a more specific/descriptive
name would be better, e.g. BURNCD_SPEED. SPEED is just too general.
--
Gabor Kovesdan
FreeBSD Volunteer
EMAIL: gabor@... .:|:. gabor@...
WEB: http://people.FreeBSD.org/~gabor .:|:. http://kovesdan.org_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
Gabor Kovesdan schrieb am 2009-11-09:
> Gabor Kovesdan escribió:
> >Alexander Best escribió:
> >>any thoughts on these small changes to burncd?
> >> - int nogap = 0, speed = 4 * 177, test_write = 0, force = 0;
> >>+ int nogap = 0, speed = 0, test_write = 0, force = 0;
> >> int block_size = 0, block_type = 0, cdopen = 0, dvdrw = 0;
> >> const char *dev;
> >> if ((dev = getenv("CDROM")) == NULL)
> >> dev = "/dev/acd0";
> >>+ if ((speed = getenv("SPEED")) == NULL)
> >>+ speed = 4 * 177;
> >>+
> >It seems incorrect. The speed variable is of type int, while getenv
> >returns char *. You should first assign getenv("SPEED") to a char *
> >variable and if it isn't NULL then you should convert it to int or
> >fall back to the default value otherwise.
> And one more thing. Personally, I think that a more
> specific/descriptive name would be better, e.g. BURNCD_SPEED. SPEED
> is just too general.
> --
> Gabor Kovesdan
> FreeBSD Volunteer
> EMAIL: gabor@... .:|:. gabor@...
> WEB: http://people.FreeBSD.org/~gabor .:|:. http://kovesdan.orgthanks for the help. how about this revised patch?
cheers.
alex
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 WRITE_SPEED"
+.It Ev WRITE_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("WRITE_SPEED")) != NULL)
+ if (strcasecmp("max", getenv) == 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':
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
Gabor Kovesdan schrieb am 2009-11-09:
> Gabor Kovesdan escribió:
> >Alexander Best escribió:
> >>any thoughts on these small changes to burncd?
> >> - int nogap = 0, speed = 4 * 177, test_write = 0, force = 0;
> >>+ int nogap = 0, speed = 0, test_write = 0, force = 0;
> >> int block_size = 0, block_type = 0, cdopen = 0, dvdrw = 0;
> >> const char *dev;
> >> if ((dev = getenv("CDROM")) == NULL)
> >> dev = "/dev/acd0";
> >>+ if ((speed = getenv("SPEED")) == NULL)
> >>+ speed = 4 * 177;
> >>+
> >It seems incorrect. The speed variable is of type int, while getenv
> >returns char *. You should first assign getenv("SPEED") to a char *
> >variable and if it isn't NULL then you should convert it to int or
> >fall back to the default value otherwise.
> And one more thing. Personally, I think that a more
> specific/descriptive name would be better, e.g. BURNCD_SPEED. SPEED
> is just too general.
> --
> Gabor Kovesdan
> FreeBSD Volunteer
> EMAIL: gabor@... .:|:. gabor@...
> WEB: http://people.FreeBSD.org/~gabor .:|:. http://kovesdan.orgooops. this one fixes the typos. ;)
alex
--- burncd.c.typo 2009-11-09 02:19:47.000000000 +0100
+++ burncd.c 2009-11-09 02:20:27.000000000 +0100
@@ -85,8 +85,8 @@
if ((dev = getenv("CDROM")) == NULL)
dev = "/dev/acd0";
- if ((env_speed = getenv("WRITE_SPEED")) != NULL)
- if (strcasecmp("max", getenv) == 0)
+ if ((env_speed = getenv("WRITE_SPEED")) != NULL) {
+ if (strcasecmp("max", env_speed) == 0)
speed = CDR_MAX_SPEED;
else
speed = atoi(env_speed) * 177;
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
On Mon, 09 Nov 2009 01:47:40 +0100 (CET), Alexander Best < alexbestms@...> wrote:
> any thoughts on these small changes to burncd?
>
> Index: usr.sbin/burncd/burncd.c
> ===================================================================
> --- usr.sbin/burncd/burncd.c (revision 199064)
> +++ usr.sbin/burncd/burncd.c (working copy)
> @@ -78,13 +78,16 @@
> {
> int arg, addr, ch, fd;
> 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 nogap = 0, speed = 0, test_write = 0, force = 0;
> int block_size = 0, block_type = 0, cdopen = 0, dvdrw = 0;
> const char *dev;
>
> if ((dev = getenv("CDROM")) == NULL)
> dev = "/dev/acd0";
>
> + if ((speed = getenv("SPEED")) == NULL)
> + speed = 4 * 177;
> +
> while ((ch = getopt(argc, argv, "def:Flmnpqs:tv")) != -1) {
> switch (ch) {
> case 'd':
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.
|
|
Re: [patch] burncd: honour for envar SPEED
On Mon, 09 Nov 2009 02:22:36 +0100 (CET), Alexander Best < alexbestms@...> wrote:
> --- burncd.c.typo 2009-11-09 02:19:47.000000000 +0100
> +++ burncd.c 2009-11-09 02:20:27.000000000 +0100
> @@ -85,8 +85,8 @@
> if ((dev = getenv("CDROM")) == NULL)
> dev = "/dev/acd0";
>
> - if ((env_speed = getenv("WRITE_SPEED")) != NULL)
> - if (strcasecmp("max", getenv) == 0)
> + if ((env_speed = getenv("WRITE_SPEED")) != NULL) {
> + if (strcasecmp("max", env_speed) == 0)
> speed = CDR_MAX_SPEED;
> else
> speed = atoi(env_speed) * 177;
atoi() doesn't really have error checking and it does not necessarily
affect `errno'. I'd probably prefer something that uses strtoul() and a
few more value/range checks, i.e.:
: #include <limits.h>
: #include <string.h>
:
: {
: char *endp;
: long xspeed;
:
: speed = 4 * 177;
: if ((env_speed = getenv("SPEED")) != NULL) {
: if (strcasecmp("max", env_speed) == 0) {
: speed = CDR_MAX_SPEED;
: } else {
: end = NULL;
: errno = 0;
: xspeed = strtol(env_speed, &endp, 0);
: if (errno != 0 || (endp != NULL && endp != str &&
: *endp != '\0' && (isdigit(*endp) != 0 ||
: isspace(*endp) == 0)))
: err(1, "invalid write speed: %s", env_speed);
: if (xspeed < 0 || xspeed > INT_MAX)
: err(1, "write speed out of range: %ld", xspeed);
: speed = (int)xspeed * 177;
: }
: }
: }
|
|
Re: [patch] burncd: honour for envar SPEED
|
|
Re: [patch] burncd: honour for envar SPEED
On Mon, 09 Nov 2009 11:00:43 +0100, Dag-Erling Smørgrav < des@...> wrote:
> Giorgos Keramidas < keramida@...> writes:
>> atoi() doesn't really have error checking and it does not necessarily
>> affect `errno'.
>
> man 3 expand_number
I know, but thanks. In this case, expand_number's logic for parsing
possible SI suffixes is not useful and may be slightly confusing.
I'm not sure what CDROM_SPEED='4m' would mean for burncd's -s option,
for example.
> And please don't call it SPEED or WRITE_SPEED or anything generic; call
> it BURNCD_SPEED or CDROM_BURN_SPEED or something unambiguous. The envar
> used to specify the device is called CDROM, not DEVICE.
Good point.
|
|
Re: [patch] burncd: honour for envar SPEED
|
|
Re: [patch] burncd: honour for envar SPEED
Giorgos Keramidas schrieb am 2009-11-09:
> On Mon, 09 Nov 2009 01:47:40 +0100 (CET), Alexander Best
> < alexbestms@...> wrote:
> > any thoughts on these small changes to burncd?
> > Index: usr.sbin/burncd/burncd.c
> > ===================================================================
> > --- usr.sbin/burncd/burncd.c (revision 199064)
> > +++ usr.sbin/burncd/burncd.c (working copy)
> > @@ -78,13 +78,16 @@
> > {
> > int arg, addr, ch, fd;
> > 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 nogap = 0, speed = 0, test_write = 0, force = 0;
> > int block_size = 0, block_type = 0, cdopen = 0, dvdrw = 0;
> > const char *dev;
> > if ((dev = getenv("CDROM")) == NULL)
> > dev = "/dev/acd0";
> > + if ((speed = getenv("SPEED")) == NULL)
> > + speed = 4 * 177;
> > +
> > while ((ch = getopt(argc, argv, "def:Flmnpqs:tv")) != -1) {
> > switch (ch) {
> > case 'd':
> 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.
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.
cheers.
alex
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
 Re: [patch] burncd: honour for envar SPEED
On Mon, 09 Nov 2009 15:28:29 +0100 (CET), Alexander Best < alexbestms@...> 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 :)
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
Giorgos Keramidas schrieb am 2009-11-09:
> On Mon, 09 Nov 2009 15:28:29 +0100 (CET), Alexander Best
> < alexbestms@...> 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
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':
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
On Mon, 09 Nov 2009 19:01:43 +0100 (CET), Alexander Best < alexbestms@...> 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':
%%%
|
|
Re: [patch] burncd: honour for envar SPEED
Giorgos Keramidas schrieb am 2009-11-09:
> On Mon, 09 Nov 2009 19:01:43 +0100 (CET), Alexander Best
> < alexbestms@...> 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
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
here's the final patch. would be great if somebody could commit this one. the
only thing i'm not 100% sure about are the burncd(8) changes. i'm not that
familiar with the man syntax.
thanks go out to keramida@ and des@ for their help.
alex
...just realised the topic makes no sense. ;) what i meant to write was
probably "[patch] burncd: honour envar SPEED". ;)
Index: usr.sbin/burncd/burncd.8
===================================================================
--- usr.sbin/burncd/burncd.8 (revision 199064)
+++ usr.sbin/burncd/burncd.8 (working copy)
@@ -27,7 +27,7 @@
.\"
.\" $FreeBSD$
.\"
-.Dd May 2, 2005
+.Dd Nov 9, 2009
.Os
.Dt BURNCD 8
.Sh NAME
@@ -158,7 +158,11 @@
.Sh ENVIRONMENT
The following environment variables affect the execution of
.Nm :
-.Bl -tag -width ".Ev CDROM"
+.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.
.It Ev CDROM
The CD device to use if one is not specified with the
.Fl f
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':
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
Alexander Best < alexbestms@...> writes:
>
> + 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':
You realize you're duplicating 6 lines of non-trivial code for no good
reason?
env_speed = getenv("BURNCD_SPEED");
while ((ch = getopt(...)) != -1) {
switch (ch) {
case 's':
env_speed = optarg;
break;
...
}
}
if (env_speed != NULL) {
if (strcasecmp...) {
...
}
}
DES
--
Dag-Erling Smørgrav - des@...
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
Dag-Erling Smørgrav schrieb am 2009-11-09:
> Alexander Best < alexbestms@...> writes:
> > + 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':
> You realize you're duplicating 6 lines of non-trivial code for no
> good
> reason?
> env_speed = getenv("BURNCD_SPEED");
> while ((ch = getopt(...)) != -1) {
> switch (ch) {
> case 's':
> env_speed = optarg;
> break;
> ...
> }
> }
> if (env_speed != NULL) {
> if (strcasecmp...) {
> ...
> }
> }
> DES
good point. is this one better?
alex
Index: usr.sbin/burncd/burncd.8
===================================================================
--- usr.sbin/burncd/burncd.8 (revision 199064)
+++ usr.sbin/burncd/burncd.8 (working copy)
@@ -27,7 +27,7 @@
.\"
.\" $FreeBSD$
.\"
-.Dd May 2, 2005
+.Dd Nov 9, 2009
.Os
.Dt BURNCD 8
.Sh NAME
@@ -158,7 +158,11 @@
.Sh ENVIRONMENT
The following environment variables affect the execution of
.Nm :
-.Bl -tag -width ".Ev CDROM"
+.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.
.It Ev CDROM
The CD device to use if one is not specified with the
.Fl f
Index: usr.sbin/burncd/burncd.c
===================================================================
--- usr.sbin/burncd/burncd.c (revision 199064)
+++ usr.sbin/burncd/burncd.c (working copy)
@@ -80,11 +80,13 @@
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";
+ env_speed = getenv("BURNCD_SPEED");
+
while ((ch = getopt(argc, argv, "def:Flmnpqs:tv")) != -1) {
switch (ch) {
case 'd':
@@ -124,12 +126,7 @@
break;
case 's':
- if (strcasecmp("max", optarg) == 0)
- speed = CDR_MAX_SPEED;
- else
- speed = atoi(optarg) * 177;
- if (speed <= 0)
- errx(EX_USAGE, "Invalid speed: %s", optarg);
+ env_speed = optarg;
break;
case 't':
@@ -147,6 +144,13 @@
argc -= optind;
argv += optind;
+ 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", optarg);
+
if (argc == 0)
usage();
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
|
Re: [patch] burncd: honour for envar SPEED
|
|
Re: [patch] burncd: honour for envar SPEED
Dag-Erling Smørgrav schrieb am 2009-11-10:
> Alexander Best < alexbestms@...> writes:
> > good point. is this one better?
> Yes (modulo indentation - run your code through tabify)
> DES
oops. found another problem. if BURNCD_SPEED and -s aren't defined strcasecmp
segfaults because env_speed is NULL.
does this patch take care of the problem?
alex
ps: would be nice if strcasecmp could protect itself from segfault with one or
both of the args being NULL.
Index: usr.sbin/burncd/burncd.8
===================================================================
--- usr.sbin/burncd/burncd.8 (revision 199125)
+++ usr.sbin/burncd/burncd.8 (working copy)
@@ -27,7 +27,7 @@
.\"
.\" $FreeBSD$
.\"
-.Dd May 2, 2005
+.Dd Nov 9, 2009
.Os
.Dt BURNCD 8
.Sh NAME
@@ -158,7 +158,11 @@
.Sh ENVIRONMENT
The following environment variables affect the execution of
.Nm :
-.Bl -tag -width ".Ev CDROM"
+.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.
.It Ev CDROM
The CD device to use if one is not specified with the
.Fl f
Index: usr.sbin/burncd/burncd.c
===================================================================
--- usr.sbin/burncd/burncd.c (revision 199125)
+++ usr.sbin/burncd/burncd.c (working copy)
@@ -80,11 +80,13 @@
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";
+ env_speed = getenv("BURNCD_SPEED");
+
while ((ch = getopt(argc, argv, "def:Flmnpqs:tv")) != -1) {
switch (ch) {
case 'd':
@@ -124,12 +126,7 @@
break;
case 's':
- if (strcasecmp("max", optarg) == 0)
- speed = CDR_MAX_SPEED;
- else
- speed = atoi(optarg) * 177;
- if (speed <= 0)
- errx(EX_USAGE, "Invalid speed: %s", optarg);
+ env_speed = optarg;
break;
case 't':
@@ -147,6 +144,15 @@
argc -= optind;
argv += optind;
+ if (env_speed == NULL)
+ ;
+ else 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", optarg);
+
if (argc == 0)
usage();
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackersTo unsubscribe, send any mail to " freebsd-hackers-unsubscribe@..."
|
attachment0 (201 bytes) 