Re: [patch] burncd: honour for envar SPEED
Alexander Best alexbes...@wwu.de writes: good point. is this one better? Yes (modulo indentation - run your code through tabify) DES -- Dag-Erling Smørgrav - d...@des.no ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
Dag-Erling Smørgrav schrieb am 2009-11-10:
Alexander Best alexbes...@wwu.de 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@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
On Tue, 10 Nov 2009, Alexander Best wrote: ps: would be nice if strcasecmp could protect itself from segfault with one or both of the args being NULL. I disagree. What do you think it should do instead? Return 0? If it did, would you have found your bug? The same argument could be made for any of the string.h functions, but I don't think it actually holds water. Such checks add overhead, and only provide an illusion of safety. Sure, strcasecmp could avoid causing the segfault itself, but at the cost of letting a broken program continue and possibly cause more damage. It could call abort(), but then you'd just have the same result (program terminates) with a different signal, and doing your check in software rather than letting the MMU hardware do it. It could print a message, but that pollutes the program's output, and 15 seconds debugging the core dump will reveal the problem anyway. Having a library function protect itself in this manner is not actually helpful, IMHO. -- Nate Eldredge n...@thatsmathematics.com ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
Nate Eldredge schrieb am 2009-11-10: On Tue, 10 Nov 2009, Alexander Best wrote: ps: would be nice if strcasecmp could protect itself from segfault with one or both of the args being NULL. I disagree. What do you think it should do instead? Return 0? If it did, would you have found your bug? The same argument could be made for any of the string.h functions, but I don't think it actually holds water. Such checks add overhead, and only provide an illusion of safety. Sure, strcasecmp could avoid causing the segfault itself, but at the cost of letting a broken program continue and possibly cause more damage. It could call abort(), but then you'd just have the same result (program terminates) with a different signal, and doing your check in software rather than letting the MMU hardware do it. It could print a message, but that pollutes the program's output, and 15 seconds debugging the core dump will reveal the problem anyway. Having a library function protect itself in this manner is not actually helpful, IMHO. -- Nate Eldredge n...@thatsmathematics.com you're right. hundreds of functions cause segfaults when arg or args are NULL. either we add safety checks for all of them (massive overhead) or just leave them the way they are. also: these functions aren't being used by regular users, but developers and it's hard finding a developer who isn't experienced with dealing with NULL pointers. ;) so problems with NULL pointers are usually fixed very quickly. alex ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
On Tue, Nov 10, 2009 at 08:03:26AM -0800, Nate Eldredge wrote: On Tue, 10 Nov 2009, Alexander Best wrote: ps: would be nice if strcasecmp could protect itself from segfault with one or both of the args being NULL. I disagree. What do you think it should do instead? Return 0? If it did, would you have found your bug? The same argument could be made for any of the string.h functions, but I don't think it actually holds water. Such checks add overhead, and only provide an illusion of safety. Sure, strcasecmp could avoid causing the segfault itself, but at the cost of letting a broken program continue and possibly cause more damage. It could call abort(), but then you'd just have the same result (program terminates) with a different signal, and doing your check in software rather than letting the MMU hardware do it. It could print a message, but that pollutes the program's output, and 15 seconds debugging the core dump will reveal the problem anyway. Having a library function protect itself in this manner is not actually helpful, IMHO. I remember System V to actually map zero page at 0, thus causing all string functions to behave like it was supplied empty string when argument is NULL. I believe Solaris still provides the library that could be LD_PRELOADed for the same effect. pgpan63mVLro1.pgp Description: PGP signature
RE: [patch] burncd: honour for envar SPEED
On Tue, Nov 10, 2009 at 08:03:26AM -0800, Nate Eldredge wrote: On Tue, 10 Nov 2009, Alexander Best wrote: ps: would be nice if strcasecmp could protect itself from segfault with one or both of the args being NULL. I disagree. What do you think it should do instead? Return 0? If it did, would you have found your bug? [snip] I remember System V to actually map zero page at 0, thus causing all string functions to behave like it was supplied empty string when argument is NULL. I believe Solaris still provides the library that could be LD_PRELOADed for the same effect. Just an anecdote: My sophomore year of undergrad (1994), I was learning C for the first time. We had to write some little thing, and on the machines in the lab my C program ran great; I had debugged it (I thought) and turned it in. The instructor took my source, compiled it on Linux, and it segfaulted when run (I think trying to print a NULL string). If HP-UX hadn't been trying to be friendly I would have been able to fully debug my code. I am sure that if I'd known then what I know now I could have made it bug free. But my experience is that a lot of code is, by necessity, written by people who aren't experts in everything they're doing. So an early segfault would have helped me in that C programming class. OTOH, if instead it had been an application someone paid money for, and it segfaulted in an untested error case instead of being graceful, I'd be mad too. Probably at the app writer, but still... Cheers, matthew ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
Alexander Best alexbes...@wwu.de writes: you're right. hundreds of functions cause segfaults when arg or args are NULL. either we add safety checks for all of them (massive overhead) or just leave them the way they are. The consensus in the C community is that adding such checks does more harm than good, because a NULL pointer is usually a symptom of a bug somewhere else in the application, and checking for a NULL pointer will either hide that bug or trigger another error somewhere down the line, possibly making the real bug harder to find, rather than easier. (next week's topic: the return value of malloc(0)...) DES -- Dag-Erling Smørgrav - d...@des.no ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
2009/11/10 Dag-Erling Smørgrav d...@des.no: Alexander Best alexbes...@wwu.de writes: you're right. hundreds of functions cause segfaults when arg or args are NULL. either we add safety checks for all of them (massive overhead) or just leave them the way they are. The consensus in the C community is that adding such checks does more harm than good, because a NULL pointer is usually a symptom of a bug somewhere else in the application, and checking for a NULL pointer will either hide that bug or trigger another error somewhere down the line, possibly making the real bug harder to find, rather than easier. And which is a way some well known OS' developers like to choose to fix sec.holes. No cookie. P.S. I apologize for flaming on this. (next week's topic: the return value of malloc(0)...) DES -- Dag-Erling Smørgrav - d...@des.no -- wbr, pluknet ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
Giorgos Keramidas keram...@ceid.upatras.gr writes: atoi() doesn't really have error checking and it does not necessarily affect `errno'. man 3 expand_number 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. DES -- Dag-Erling Smørgrav - d...@des.no ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
On Mon, 09 Nov 2009 11:00:43 +0100, Dag-Erling Smørgrav d...@des.no wrote: Giorgos Keramidas keram...@ceid.upatras.gr 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. pgp27FxReZQoN.pgp Description: PGP signature
Re: [patch] burncd: honour for envar SPEED
Giorgos Keramidas keram...@freebsd.org writes: Dag-Erling Smørgrav d...@des.no wrote: 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. Hmm, I thought the speed was expressed in kBps or something... DES -- Dag-Erling Smørgrav - d...@des.no ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
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
alexbes...@math.uni-muenster.de 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@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
On Mon, 09 Nov 2009 15:28:29 +0100 (CET), Alexander Best alexbes...@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 :)
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
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
alexbes...@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
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@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
On Mon, 09 Nov 2009 19:01:43 +0100 (CET), Alexander Best alexbes...@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':
%%%
pgp5BQPCQ7Jo2.pgp
Description: PGP signature
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
alexbes...@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
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
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@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
Alexander Best alexbes...@wwu.de 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 - d...@des.no
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
Dag-Erling Smørgrav schrieb am 2009-11-09:
Alexander Best alexbes...@wwu.de 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@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
[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@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
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: ga...@freebsd.org .:|:. ga...@kovesdan.org WEB: http://people.FreeBSD.org/~gabor .:|:. http://kovesdan.org ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
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: ga...@freebsd.org .:|:. ga...@kovesdan.org WEB: http://people.FreeBSD.org/~gabor .:|:. http://kovesdan.org ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
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: ga...@freebsd.org .:|:. ga...@kovesdan.org
WEB: http://people.FreeBSD.org/~gabor .:|:. http://kovesdan.org
thanks 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@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
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: ga...@freebsd.org .:|:. ga...@kovesdan.org
WEB: http://people.FreeBSD.org/~gabor .:|:. http://kovesdan.org
ooops. this one fixes the typos. ;)
alex
--- burncd.c.typo 2009-11-09 02:19:47.0 +0100
+++ burncd.c2009-11-09 02:20:27.0 +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@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: [patch] burncd: honour for envar SPEED
On Mon, 09 Nov 2009 01:47:40 +0100 (CET), Alexander Best
alexbes...@math.uni-muenster.de 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.
pgpl1Dl1ze7nw.pgp
Description: PGP signature
Re: [patch] burncd: honour for envar SPEED
On Mon, 09 Nov 2009 02:22:36 +0100 (CET), Alexander Best
alexbes...@math.uni-muenster.de wrote:
--- burncd.c.typo 2009-11-09 02:19:47.0 +0100
+++ burncd.c 2009-11-09 02:20:27.0 +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;
: }
: }
: }
pgpIw3IMiex7c.pgp
Description: PGP signature
