[patch] burncd: honour for envar SPEED
Gabor Kovesdan
> any thoughts on these small changes to burncd?
>
> + 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...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hacke...@freebsd.org"
Gabor Kovesdan
> 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.
name would be better, e.g. BURNCD_SPEED. SPEED is just too general.
Giorgos Keramidas
> any thoughts on these small changes to burncd?
>
> ===================================================================
> --- 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;
> +
> 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.
Giorgos Keramidas
> --- 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 (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;
: }
: }
: }
Dag-Erling Smørgrav
> 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
Giorgos Keramidas
> Giorgos Keramidas <kera...@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.
Dag-Erling Smørgrav
> 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...
Alexander Best
> Hi Alexander,
> env SPEED=12234567890 burncd ...
> env SPEED=4 burncd
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
Giorgos Keramidas
> 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 :)
Alexander Best
> >> env SPEED=12234567890 burncd ...
> >> env SPEED=4 burncd
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
Giorgos Keramidas
>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.
>
>> 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':
%%%
Alexander Best
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
Alexander Best
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". ;)
Dag-Erling Smørgrav
>
> + 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
Dag-Erling Smørgrav
> good point. is this one better?
Yes (modulo indentation - run your code through tabify)
Alexander Best
> Alexander Best <alexb...@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.
Nate Eldredge
> 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
na...@thatsmathematics.com
Alexander Best
> 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
> na...@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
Kostik Belousov
> 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.
Matthew Fleming
> > 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?
> >
>
> 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
pluknet
> Alexander Best <alexb...@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.
>
> 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