Re: GEOM/fdisk/USB drive problem

2002-10-15 Thread Poul-Henning Kamp

In message [EMAIL PROTECTED], Nate Lawson wri
tes:
fdisk against my USB flash drive crashes with divide by zero.  It turns
out that get_params() starts with some default values (since there is
no disklabel) and then runs some ioctls: DIOCGFWSECTORS and DIOCGFWHEADS.

The problem is that fdisk checks the error from that ioctl and then
happily uses whatever parameters it returned.  My question is, should I
add the error checking in userland (error == 0  sector != 0) or in
g_dev_ioctl?  IMO, it should be in g_dev_ioctl so that we only have to
validate the data in one place instead of multiple utilities (fdisk,
disklabel, ...?)

I really think it belongs in userland and not in the kernel.

The FW values are advisory, and if they are not there or not
sensible, userland should cope.

I could agree to make g_dev_ioctl fail the ioctl with some errno
if they came back as zero, but not substituting another value.

-- 
Poul-Henning Kamp   | UNIX since Zilog Zeus 3.20
[EMAIL PROTECTED] | TCP/IP since RFC 956
FreeBSD committer   | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.

To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message



Re: GEOM/fdisk/USB drive problem

2002-10-15 Thread Nate Lawson

On Tue, 15 Oct 2002, Poul-Henning Kamp wrote:
 In message [EMAIL PROTECTED], Nate Lawson wri
 tes:
 fdisk against my USB flash drive crashes with divide by zero.  It turns
 out that get_params() starts with some default values (since there is
 no disklabel) and then runs some ioctls: DIOCGFWSECTORS and DIOCGFWHEADS.
 
 The problem is that fdisk checks the error from that ioctl and then
 happily uses whatever parameters it returned.  My question is, should I
 add the error checking in userland (error == 0  sector != 0) or in
 g_dev_ioctl?  IMO, it should be in g_dev_ioctl so that we only have to
 validate the data in one place instead of multiple utilities (fdisk,
 disklabel, ...?)
 
 I really think it belongs in userland and not in the kernel.
 
 The FW values are advisory, and if they are not there or not
 sensible, userland should cope.
 
 I could agree to make g_dev_ioctl fail the ioctl with some errno
 if they came back as zero, but not substituting another value.

The attached patch makes everything work fine for my drive (64 MB USB
flash).

-Nate

fdisk: can't get disk parameters on /dev/da0; supplying dummy ones
*** Working on device /dev/da0 ***
parameters extracted from in-core disklabel are:
cylinders=129024 heads=1 sectors/track=1 (1 blks/cyl)

Figures below won't work with BIOS for partitions not in cyl 1
parameters to be used for BIOS calculations are:
cylinders=129024 heads=1 sectors/track=1 (1 blks/cyl)

Media sector size is 512
Warning: BIOS sector numbering starts with sector 1
Information from DOS bootblock is:
The data for partition 1 is:
sysid 6 (0x06),(Primary 'big' DOS (= 32MB))
start 32, size 128480 (62 Meg), flag 80 (active)
beg: cyl 0/ head 1/ sector 1;
end: cyl 250/ head 15/ sector 32
The data for partition 2 is:
UNUSED
The data for partition 3 is:
UNUSED
The data for partition 4 is:
UNUSED



Index: sys/geom/geom_dev.c
===
RCS file: /home/ncvs/src/sys/geom/geom_dev.c,v
retrieving revision 1.25
diff -u -r1.25 geom_dev.c
--- sys/geom/geom_dev.c 7 Oct 2002 06:25:26 -   1.25
+++ sys/geom/geom_dev.c 15 Oct 2002 12:46:35 -
@@ -245,15 +245,23 @@
switch (cmd) {
case DIOCGSECTORSIZE:
error = g_io_getattr(GEOM::sectorsize, cp, i, data);
+   if (error == 0  *(u_int *)data == 0)
+   error = EINVAL;
break;
case DIOCGMEDIASIZE:
error = g_io_getattr(GEOM::mediasize, cp, i, data);
+   if (error == 0  *(u_int *)data == 0)
+   error = EINVAL;
break;
case DIOCGFWSECTORS:
error = g_io_getattr(GEOM::fwsectors, cp, i, data);
+   if (error == 0  *(u_int *)data == 0)
+   error = EINVAL;
break;
case DIOCGFWHEADS:
error = g_io_getattr(GEOM::fwheads, cp, i, data);
+   if (error == 0  *(u_int *)data == 0)
+   error = EINVAL;
break;
case DIOCGFRONTSTUFF:
error = g_io_getattr(GEOM::frontstuff, cp, i, data);



Re: GEOM/fdisk/USB drive problem

2002-10-15 Thread Poul-Henning Kamp

In message [EMAIL PROTECTED], Nate Lawson wri
tes:

 I could agree to make g_dev_ioctl fail the ioctl with some errno
 if they came back as zero, but not substituting another value.

The attached patch makes everything work fine for my drive (64 MB USB
flash).

Apart from the choice of EINVAL, this would be OK with me, I would
probably say ENOENT instead.  EINVAL generally means you gave a
wrong parameter to a syscall,  ENOENT is more of a Looked, found
nothing kind of error.

-- 
Poul-Henning Kamp   | UNIX since Zilog Zeus 3.20
[EMAIL PROTECTED] | TCP/IP since RFC 956
FreeBSD committer   | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.

To Unsubscribe: send mail to [EMAIL PROTECTED]
with unsubscribe freebsd-current in the body of the message