Linus,
Here's the latest set of device mapper patches against 2.5.63. All
trivial. Please apply.
Thanks,
- Joe
When reloading a device the ioctl interface was forgetting to drop a
reference on the new table.
--- diff/drivers/md/dm-ioctl.c 2003-01-13 14:18:15.000000000 +0000
+++ source/drivers/md/dm-ioctl.c 2003-02-26 16:09:42.000000000 +0000
@@ -887,6 +887,7 @@
dm_table_put(t);
return r;
}
+ dm_table_put(t); /* md will have taken its own reference */
set_disk_ro(dm_disk(md), (param->flags & DM_READONLY_FLAG));
dm_put(md);
Silly mistake in error path when an unknown target type is requested.
--- diff/drivers/md/dm-table.c 2003-02-26 16:09:47.000000000 +0000
+++ source/drivers/md/dm-table.c 2003-02-26 16:10:02.000000000 +0000
@@ -591,7 +591,7 @@
tgt->type = dm_get_target_type(type);
if (!tgt->type) {
tgt->error = "unknown target type";
- goto bad;
+ return r;
}
tgt->table = t;
Another fix for the __LOW macro.
When dm_table and dm_target structures are initialized, the "limits" fields
(struct io_restrictions) are initialized to zero (e.g. in dm_table_add_target()
in dm-table.c). However, zero is not a useable value in these fields. The
request queue will never let an I/O through, regardless of how small it might
be, if max_sectors is set to zero (see generic_make_request in ll_rw_blk.c).
This change to the __LOW() macro sets these fields correctly when they are
first initialized. [Kevin Corry]
--- diff/drivers/md/dm-table.c 2003-02-26 16:10:02.000000000 +0000
+++ source/drivers/md/dm-table.c 2003-02-26 16:10:19.000000000 +0000
@@ -79,7 +79,7 @@
}
#define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
-#define __LOW(l, r) if (*(l) > (r)) *(l) = (r)
+#define __LOW(l, r) if (*(l) == 0 || *(l) > (r)) *(l) = (r)
/*
* Combine two io_restrictions, always taking the lower value.
Use the correct size for "name" in register_with_devfs().
During Al Viro's devfs cleanup a few versions ago, this function was
rewritten, and the "name" string added. The 32-byte size is not large
enough to prevent a possible buffer overflow in the sprintf() call,
since the hash cell can have a name up to 128 characters.
[Kevin Corry]
--- diff/drivers/md/dm-ioctl.c 2003-02-26 16:09:42.000000000 +0000
+++ source/drivers/md/dm-ioctl.c 2003-02-26 16:09:52.000000000 +0000
@@ -173,7 +173,7 @@
*/
static int register_with_devfs(struct hash_cell *hc)
{
- char name[32];
+ char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
struct gendisk *disk = dm_disk(hc->md);
sprintf(name, DM_DIR "/%s", hc->name);
Fix __LOW macro. [Kevin Corry]
--- diff/drivers/md/dm-table.c 2003-01-13 14:18:15.000000000 +0000
+++ source/drivers/md/dm-table.c 2003-02-26 16:09:47.000000000 +0000
@@ -79,7 +79,7 @@
}
#define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
-#define __LOW(l, r) if (*(l) < (r)) *(l) = (r)
+#define __LOW(l, r) if (*(l) > (r)) *(l) = (r)
/*
* Combine two io_restrictions, always taking the lower value.
Fix problem with devfs when unloading the dm module.
dm-ioctl.c: deregister the misc device, and its associated symlink
*before* removing the /dev/mapper dir. [Alasdair Kergon]
--- diff/drivers/md/dm-ioctl.c 2003-02-26 16:09:52.000000000 +0000
+++ source/drivers/md/dm-ioctl.c 2003-02-26 16:09:57.000000000 +0000
@@ -1123,17 +1123,17 @@
return 0;
failed:
+ devfs_remove(DM_DIR "/control");
+ if (misc_deregister(&_dm_misc) < 0)
+ DMERR("misc_deregister failed for control device");
dm_hash_exit();
- misc_deregister(&_dm_misc);
return r;
}
void dm_interface_exit(void)
{
- dm_hash_exit();
-
devfs_remove(DM_DIR "/control");
-
if (misc_deregister(&_dm_misc) < 0)
DMERR("misc_deregister failed for control device");
+ dm_hash_exit();
}
Return correct error codes from dm_table_add_target(). [Kevin Corry]
--- diff/drivers/md/dm-table.c 2003-02-26 16:10:19.000000000 +0000
+++ source/drivers/md/dm-table.c 2003-02-26 16:10:24.000000000 +0000
@@ -591,7 +591,7 @@
tgt->type = dm_get_target_type(type);
if (!tgt->type) {
tgt->error = "unknown target type";
- return r;
+ return -EINVAL;
}
tgt->table = t;
@@ -604,6 +604,7 @@
*/
if (!adjoin(t, tgt)) {
tgt->error = "Gap in table";
+ r = -EINVAL;
goto bad;
}
Allow slashes ('/') within a DM device name, but not at the beginning.
Devfs will automatically create all necessary sub-directories if a name
with embedded slashes is registered. [Kevin Corry]
--- diff/drivers/md/dm-ioctl.c 2003-02-26 16:09:57.000000000 +0000
+++ source/drivers/md/dm-ioctl.c 2003-02-26 16:10:07.000000000 +0000
@@ -545,7 +545,7 @@
static int check_name(const char *name)
{
- if (strchr(name, '/')) {
+ if (name[0] == '/') {
DMWARN("invalid device name");
return -EINVAL;
}
On Wed, Feb 26, 2003 at 05:11:57PM +0000, Joe Thornber wrote:
> Allow slashes ('/') within a DM device name, but not at the beginning.
>
> Devfs will automatically create all necessary sub-directories if a name
> with embedded slashes is registered. [Kevin Corry]
Does this interact with the block device name representation in sysfs?
I don't think sysfs can handle names with '/' very well.
thanks,
greg k-h
On Wednesday 26 February 2003 11:38, Greg KH wrote:
> On Wed, Feb 26, 2003 at 05:11:57PM +0000, Joe Thornber wrote:
> > Allow slashes ('/') within a DM device name, but not at the beginning.
> >
> > Devfs will automatically create all necessary sub-directories if a name
> > with embedded slashes is registered. [Kevin Corry]
>
> Does this interact with the block device name representation in sysfs?
> I don't think sysfs can handle names with '/' very well.
This patch shouldn't affect DM's interaction with sysfs.
The names in sysfs are generated by the DM core, and are all called "dm-x",
where x is the device minor number.
The names referred to in the above patch are used only by the device-mapper
ioctl interface, and are not seen by the DM core. These are the names
generated by the user-space tools, and are the ones that are passed to devfs,
which doesn't have a problem with embedded slashes.
--
Kevin Corry
[email protected]
http://evms.sourceforge.net/
On Wed, Feb 26, 2003 at 12:17:12PM -0600, Kevin Corry wrote:
> On Wednesday 26 February 2003 11:38, Greg KH wrote:
> > On Wed, Feb 26, 2003 at 05:11:57PM +0000, Joe Thornber wrote:
> > > Allow slashes ('/') within a DM device name, but not at the beginning.
> > >
> > > Devfs will automatically create all necessary sub-directories if a name
> > > with embedded slashes is registered. [Kevin Corry]
> >
> > Does this interact with the block device name representation in sysfs?
> > I don't think sysfs can handle names with '/' very well.
>
> This patch shouldn't affect DM's interaction with sysfs.
>
> The names in sysfs are generated by the DM core, and are all called "dm-x",
> where x is the device minor number.
Ok, but it sure would be nice to see the "name" in sysfs instead of the
minor number, right? Just thinking of how this would affect my
much-out-of-date sysfs dm patches :)
thanks,
greg k-h
On Wed, Feb 26, 2003 at 05:12:49PM +0000, Joe Thornber wrote:
> Another fix for the __LOW macro.
>
> When dm_table and dm_target structures are initialized, the "limits" fields
> (struct io_restrictions) are initialized to zero (e.g. in dm_table_add_target()
> in dm-table.c). However, zero is not a useable value in these fields. The
> request queue will never let an I/O through, regardless of how small it might
> be, if max_sectors is set to zero (see generic_make_request in ll_rw_blk.c).
> This change to the __LOW() macro sets these fields correctly when they are
> first initialized. [Kevin Corry]
>
> --- diff/drivers/md/dm-table.c 2003-02-26 16:10:02.000000000 +0000
> +++ source/drivers/md/dm-table.c 2003-02-26 16:10:19.000000000 +0000
> @@ -79,7 +79,7 @@
> }
>
> #define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
> -#define __LOW(l, r) if (*(l) > (r)) *(l) = (r)
> +#define __LOW(l, r) if (*(l) == 0 || *(l) > (r)) *(l) = (r)
Any reason to not use the existing min() and max() macros instead of
these? Then:
__HIGH(foo, bar);
can be written as:
foo = max(foo, bar);
which is (IMHO) easier to read.
By special casing the logic in your __LOW() macro, you're only asking
for trouble in the long run :)
thanks,
greg k-h
On Wed, Feb 26, 2003 at 10:14:54AM -0800, Greg KH wrote:
> By special casing the logic in your __LOW() macro, you're only asking
> for trouble in the long run :)
I think you're right, the __HIGH and __LOW macros are just obfuscating
things. I'll fix in the next patchset.
Thanks,
- Joe
On Wed, Feb 26, 2003 at 05:12:49PM +0000, Joe Thornber wrote:
> Another fix for the __LOW macro.
>
> When dm_table and dm_target structures are initialized, the "limits" fields
> (struct io_restrictions) are initialized to zero (e.g. in dm_table_add_target()
> in dm-table.c). However, zero is not a useable value in these fields. The
> request queue will never let an I/O through, regardless of how small it might
> be, if max_sectors is set to zero (see generic_make_request in ll_rw_blk.c).
> This change to the __LOW() macro sets these fields correctly when they are
> first initialized. [Kevin Corry]
>
> --- diff/drivers/md/dm-table.c 2003-02-26 16:10:02.000000000 +0000
> +++ source/drivers/md/dm-table.c 2003-02-26 16:10:19.000000000 +0000
> @@ -79,7 +79,7 @@
> }
>
> #define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
> -#define __LOW(l, r) if (*(l) > (r)) *(l) = (r)
> +#define __LOW(l, r) if (*(l) == 0 || *(l) > (r)) *(l) = (r)
As someone else suggested, it would be good style to just use the
existing min() and max() macros.
Special-casing like the above is a recipe for disaster - having a
macro named "__LOW" which actually translates to "min() unless left
argument is zero in which case we do a max() instead" is going to get
someone in trouble one day.
I'd say use "min()" and if there are places in the code where you want
min() unless that will be zero, then make that condition *explicit* at
those places in the code.
If you want a magic "usually_min_but_sometimes_max()" macro, then make
it's *name* reflect it's voodoo propeties.
Just my 0.02 Euro
--
................................................................
: [email protected] : And I see the elder races, :
:.........................: putrid forms of man :
: Jakob ?stergaard : See him rise and claim the earth, :
: OZ9ABN : his downfall is at hand. :
:.........................:............{Konkhra}...............:
Greg,
Any happier with this ? The second hunk of the patch may disappear at
some point.
- Joe
Replace __HIGH() and __LOW() with max() and min_not_zero().
--- diff/drivers/md/dm-table.c 2003-02-26 16:10:24.000000000 +0000
+++ source/drivers/md/dm-table.c 2003-02-27 09:44:31.000000000 +0000
@@ -78,22 +78,33 @@
return result;
}
-#define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
-#define __LOW(l, r) if (*(l) == 0 || *(l) > (r)) *(l) = (r)
+/*
+ * Returns the minimum that is _not_ zero, unless both are zero.
+ */
+#define min_not_zero(l, r) (l == 0) ? r : ((r == 0) ? l : min(l, r))
/*
* Combine two io_restrictions, always taking the lower value.
*/
-
static void combine_restrictions_low(struct io_restrictions *lhs,
struct io_restrictions *rhs)
{
- __LOW(&lhs->max_sectors, rhs->max_sectors);
- __LOW(&lhs->max_phys_segments, rhs->max_phys_segments);
- __LOW(&lhs->max_hw_segments, rhs->max_hw_segments);
- __HIGH(&lhs->hardsect_size, rhs->hardsect_size);
- __LOW(&lhs->max_segment_size, rhs->max_segment_size);
- __LOW(&lhs->seg_boundary_mask, rhs->seg_boundary_mask);
+ lhs->max_sectors =
+ min_not_zero(lhs->max_sectors, rhs->max_sectors);
+
+ lhs->max_phys_segments =
+ min_not_zero(lhs->max_phys_segments, rhs->max_phys_segments);
+
+ lhs->max_hw_segments =
+ min_not_zero(lhs->max_hw_segments, rhs->max_hw_segments);
+
+ lhs->hardsect_size = max(lhs->hardsect_size, rhs->hardsect_size);
+
+ lhs->max_segment_size =
+ min_not_zero(lhs->max_segment_size, rhs->max_segment_size);
+
+ lhs->seg_boundary_mask =
+ min_not_zero(lhs->seg_boundary_mask, rhs->seg_boundary_mask);
}
/*
@@ -486,13 +497,31 @@
request_queue_t *q = bdev_get_queue((*result)->bdev);
struct io_restrictions *rs = &ti->limits;
- /* combine the device limits low */
- __LOW(&rs->max_sectors, q->max_sectors);
- __LOW(&rs->max_phys_segments, q->max_phys_segments);
- __LOW(&rs->max_hw_segments, q->max_hw_segments);
- __HIGH(&rs->hardsect_size, q->hardsect_size);
- __LOW(&rs->max_segment_size, q->max_segment_size);
- __LOW(&rs->seg_boundary_mask, q->seg_boundary_mask);
+ /*
+ * Combine the device limits low.
+ *
+ * FIXME: if we move an io_restriction struct
+ * into q this would just be a call to
+ * combine_restrictions_low()
+ */
+ rs->max_sectors =
+ min_not_zero(rs->max_sectors, q->max_sectors);
+
+ rs->max_phys_segments =
+ min_not_zero(rs->max_phys_segments,
+ q->max_phys_segments);
+
+ rs->max_hw_segments =
+ min_not_zero(rs->max_hw_segments, q->max_hw_segments);
+
+ rs->hardsect_size = max(rs->hardsect_size, q->hardsect_size);
+
+ rs->max_segment_size =
+ min_not_zero(rs->max_segment_size, q->max_segment_size);
+
+ rs->seg_boundary_mask =
+ min_not_zero(rs->seg_boundary_mask,
+ q->seg_boundary_mask);
}
return r;
On Thu, Feb 27, 2003 at 10:34:42AM +0100, Jakob Oestergaard wrote:
> If you want a magic "usually_min_but_sometimes_max()" macro, then make
> it's *name* reflect it's voodoo propeties.
See the patch I just posted in response to Greg.
- Joe
Joe Thornber <[email protected]> said:
> Use the correct size for "name" in register_with_devfs().
>
> During Al Viro's devfs cleanup a few versions ago, this function was
> rewritten, and the "name" string added. The 32-byte size is not large
> enough to prevent a possible buffer overflow in the sprintf() call,
> since the hash cell can have a name up to 128 characters.
>
> [Kevin Corry]
>
> --- diff/drivers/md/dm-ioctl.c 2003-02-26 16:09:42.000000000 +0000
> +++ source/drivers/md/dm-ioctl.c 2003-02-26 16:09:52.000000000 +0000
> @@ -173,7 +173,7 @@
> */
> static int register_with_devfs(struct hash_cell *hc)
> {
> - char name[32];
> + char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
This either makes a large name array or generates a possibly huge array at
runtime (bad if your stack is < 8KiB).
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
On Thu, Feb 27, 2003 at 10:01:44AM +0000, Joe Thornber wrote:
> On Thu, Feb 27, 2003 at 10:34:42AM +0100, Jakob Oestergaard wrote:
> > If you want a magic "usually_min_but_sometimes_max()" macro, then make
> > it's *name* reflect it's voodoo propeties.
>
> See the patch I just posted in response to Greg.
That makes me happy, for all it matters ;)
Cheers,
--
................................................................
: [email protected] : And I see the elder races, :
:.........................: putrid forms of man :
: Jakob ?stergaard : See him rise and claim the earth, :
: OZ9ABN : his downfall is at hand. :
:.........................:............{Konkhra}...............:
On Wednesday 26 February 2003 15:04, Horst von Brand wrote:
> Joe Thornber <[email protected]> said:
> > Use the correct size for "name" in register_with_devfs().
> >
> > During Al Viro's devfs cleanup a few versions ago, this function was
> > rewritten, and the "name" string added. The 32-byte size is not large
> > enough to prevent a possible buffer overflow in the sprintf() call,
> > since the hash cell can have a name up to 128 characters.
> >
> > [Kevin Corry]
> >
> > --- diff/drivers/md/dm-ioctl.c 2003-02-26 16:09:42.000000000 +0000
> > +++ source/drivers/md/dm-ioctl.c 2003-02-26 16:09:52.000000000 +0000
> > @@ -173,7 +173,7 @@
> > */
> > static int register_with_devfs(struct hash_cell *hc)
> > {
> > - char name[32];
> > + char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
>
> This either makes a large name array or generates a possibly huge array at
> runtime (bad if your stack is < 8KiB).
Would this be better?
--
Kevin Corry
[email protected]
http://evms.sourceforge.net/
--- linux-2.5.60a/drivers/md/dm-ioctl.c 2003/02/13 16:43:26
+++ linux-2.5.60b/drivers/md/dm-ioctl.c 2003/02/27 14:17:00
@@ -173,8 +173,11 @@
*/
static int register_with_devfs(struct hash_cell *hc)
{
- char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
struct gendisk *disk = dm_disk(hc->md);
+ char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
+ if (!name) {
+ return -ENOMEM;
+ }
sprintf(name, DM_DIR "/%s", hc->name);
devfs_register(NULL, name, DEVFS_FL_CURRENT_OWNER,
On Thursday 27 February 2003 08:20, Kevin Corry wrote:
> On Wednesday 26 February 2003 15:04, Horst von Brand wrote:
> > Joe Thornber <[email protected]> said:
> > > Use the correct size for "name" in register_with_devfs().
> > >
> > > During Al Viro's devfs cleanup a few versions ago, this function was
> > > rewritten, and the "name" string added. The 32-byte size is not large
> > > enough to prevent a possible buffer overflow in the sprintf() call,
> > > since the hash cell can have a name up to 128 characters.
> > >
> > > [Kevin Corry]
> > >
> > > --- diff/drivers/md/dm-ioctl.c 2003-02-26 16:09:42.000000000 +0000
> > > +++ source/drivers/md/dm-ioctl.c 2003-02-26 16:09:52.000000000 +0000
> > > @@ -173,7 +173,7 @@
> > > */
> > > static int register_with_devfs(struct hash_cell *hc)
> > > {
> > > - char name[32];
> > > + char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
> >
> > This either makes a large name array or generates a possibly huge array
> > at runtime (bad if your stack is < 8KiB).
>
> Would this be better?
As Joe pointed out to me, that should have been:
--- linux-2.5.60a/drivers/md/dm-ioctl.c 2003/02/13 16:43:26
+++ linux-2.5.60b/drivers/md/dm-ioctl.c 2003/02/27 14:35:20
@@ -173,14 +173,18 @@
*/
static int register_with_devfs(struct hash_cell *hc)
{
- char name[DM_NAME_LEN + strlen(DM_DIR) + 1];
struct gendisk *disk = dm_disk(hc->md);
+ char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
+ if (!name) {
+ return -ENOMEM;
+ }
sprintf(name, DM_DIR "/%s", hc->name);
devfs_register(NULL, name, DEVFS_FL_CURRENT_OWNER,
disk->major, disk->first_minor,
S_IFBLK | S_IRUSR | S_IWUSR | S_IRGRP,
&dm_blk_dops, NULL);
+ kfree(name);
return 0;
}
Joe Thornber <[email protected]> said:
> Any happier with this ? The second hunk of the patch may disappear at
> some point.
>
> - Joe
>
>
> Replace __HIGH() and __LOW() with max() and min_not_zero().
>
>
> --- diff/drivers/md/dm-table.c 2003-02-26 16:10:24.000000000 +0000
> +++ source/drivers/md/dm-table.c 2003-02-27 09:44:31.000000000 +0000
> @@ -78,22 +78,33 @@
> return result;
> }
>
> -#define __HIGH(l, r) if (*(l) < (r)) *(l) = (r)
> -#define __LOW(l, r) if (*(l) == 0 || *(l) > (r)) *(l) = (r)
> +/*
> + * Returns the minimum that is _not_ zero, unless both are zero.
> + */
> +#define min_not_zero(l, r) (l == 0) ? r : ((r == 0) ? l : min(l, r))
I'd add () around r and l just for paranoia's sake. Plus make sure they
aren't ever going to be called with sideeffects... why not inline functions?
Besides, I'd call them args a and b (no real reason for l and r, and l is
almost 1 for some fonts...)
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
> + char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
> + if (!name) {
> + return -ENOMEM;
> + }
Also, kmalloc() needs a second "GFP_xxx" parameter (I guess GFP_KERNEL
in this case, although I don't know the context this function is
called from).
- Roland
On Thu, Feb 27, 2003 at 01:17:05PM -0300, Horst von Brand wrote:
> I'd add () around r and l just for paranoia's sake. Plus make sure they
> aren't ever going to be called with sideeffects... why not inline functions?
because the types vary :(
> Besides, I'd call them args a and b (no real reason for l and r, and l is
> almost 1 for some fonts...)
l and r stood for left and right from when it was a compare and assign
operator.
- Joe
On Thu, Feb 27, 2003 at 09:55:22AM +0000, Joe Thornber wrote:
> Greg,
>
> Any happier with this ? The second hunk of the patch may disappear at
> some point.
Much nicer, thanks.
greg k-h
[Sent this earlier, but it doesn't seem to have shown up yet. My outgoing
email server seems to flake out at times. Sorry if this is a duplicate, but
it seemed important enough to resend, since we'd like to avoid the extra
compile failure.]
On Thursday 27 February 2003 10:25, Roland Dreier wrote:
> > + char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
> > + if (!name) {
> > + return -ENOMEM;
> > + }
>
> Also, kmalloc() needs a second "GFP_xxx" parameter (I guess GFP_KERNEL
> in this case, although I don't know the context this function is
> called from).
>
> - Roland
Dammit! I'm not having a good morning. :(
Unfortunately, Linus seems to have committed that patch already. So here is a
patch to fix just that line.
Thanks for catching that.
--
Kevin Corry
[email protected]
http://evms.sourceforge.net/
--- a/drivers/md/dm-ioctl.c 2003/02/27 16:29:58
+++ b/drivers/md/dm-ioctl.c 2003/02/27 17:21:54
@@ -174,7 +174,7 @@
static int register_with_devfs(struct hash_cell *hc)
{
struct gendisk *disk = dm_disk(hc->md);
- char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
+ char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1, GFP_KERNEL);
if (!name) {
return -ENOMEM;
}
On Thursday 27 February 2003 10:25, Roland Dreier wrote:
> > + char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
> > + if (!name) {
> > + return -ENOMEM;
> > + }
>
> Also, kmalloc() needs a second "GFP_xxx" parameter (I guess GFP_KERNEL
> in this case, although I don't know the context this function is
> called from).
>
> - Roland
Dammit! I'm not having a good morning. :(
Unfortunately, Linus seems to have committed that patch already. So here is a
patch to fix just that line.
Thanks for catching that.
--
Kevin Corry
[email protected]
http://evms.sourceforge.net/
--- a/drivers/md/dm-ioctl.c 2003/02/27 16:29:58
+++ b/drivers/md/dm-ioctl.c 2003/02/27 16:30:03
@@ -174,7 +174,7 @@
static int register_with_devfs(struct hash_cell *hc)
{
struct gendisk *disk = dm_disk(hc->md);
- char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
+ char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1, GFP_KERNEL);
if (!name) {
return -ENOMEM;
}
On Friday 28 February 2003 08:32, you wrote:
> On Thu, 2003-02-27 at 14:05, Kevin Corry wrote:
> > Unfortunately, Linus seems to have committed that patch already. So here
> > is a patch to fix just that line.
> >
> > Thanks for catching that.
>
> Third time, strlen isn't necessary, it can be done at compile time.
>
> --- a/drivers/md/dm-ioctl.c 2003/02/27 16:29:58
> +++ b/drivers/md/dm-ioctl.c 2003/02/27 17:21:54
> @@ -174,7 +174,7 @@
> static int register_with_devfs(struct hash_cell *hc)
> {
> struct gendisk *disk = dm_disk(hc->md);
> - char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
> + char *name = kmalloc(DM_NAME_LEN + sizeof(DM_DIR));
> if (!name) {
> return -ENOMEM;
> }
Sorry, I sent the last patch before I got your email.
Also, the "+1" is still necessary, even if we switch to sizeof. The sprintf
call that follows copies DM_DIR, followed by a slash, followed by the name
from the hash table into the allocated string. The "+1" is for the slash in
the middle. The terminating NULL character is accounted for in DM_NAME_LEN.
Linus, here is (yet another!) patch against current BK.
--
Kevin Corry
[email protected]
http://evms.sourceforge.net/
--- linux-2.5.63-bk4a/drivers/md/dm-ioctl.c Fri Feb 28 08:43:19 2003
+++ linux-2.5.63-bk4b/drivers/md/dm-ioctl.c Fri Feb 28 08:44:08 2003
@@ -174,7 +174,7 @@
static int register_with_devfs(struct hash_cell *hc)
{
struct gendisk *disk = dm_disk(hc->md);
- char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1, GFP_KERNEL);
+ char *name = kmalloc(DM_NAME_LEN + sizeof(DM_DIR) + 1, GFP_KERNEL);
if (!name) {
return -ENOMEM;
}
Kevin Corry <[email protected]> said:
> On Friday 28 February 2003 08:32, you wrote:
> > On Thu, 2003-02-27 at 14:05, Kevin Corry wrote:
> > > Unfortunately, Linus seems to have committed that patch already. So here
> > > is a patch to fix just that line.
> > >
> > > Thanks for catching that.
> >
> > Third time, strlen isn't necessary, it can be done at compile time.
> >
> > --- a/drivers/md/dm-ioctl.c 2003/02/27 16:29:58
> > +++ b/drivers/md/dm-ioctl.c 2003/02/27 17:21:54
> > @@ -174,7 +174,7 @@
> > static int register_with_devfs(struct hash_cell *hc)
> > {
> > struct gendisk *disk = dm_disk(hc->md);
> > - char *name = kmalloc(DM_NAME_LEN + strlen(DM_DIR) + 1);
> > + char *name = kmalloc(DM_NAME_LEN + sizeof(DM_DIR));
> > if (!name) {
> > return -ENOMEM;
> > }
>
> Sorry, I sent the last patch before I got your email.
>
> Also, the "+1" is still necessary, even if we switch to sizeof. The sprintf
> call that follows copies DM_DIR, followed by a slash, followed by the name
> from the hash table into the allocated string. The "+1" is for the slash in
> the middle. The terminating NULL character is accounted for in
> DM_NAME_LEN.
Then it was broken before.
sizeof("1234") == strlen("1234") + 1 == 5
On Friday 28 February 2003 12:14, Horst von Brand wrote:
> Kevin Corry <[email protected]> said:
> > Also, the "+1" is still necessary, even if we switch to sizeof. The
> > sprintf call that follows copies DM_DIR, followed by a slash, followed by
> > the name from the hash table into the allocated string. The "+1" is for
> > the slash in the middle. The terminating NULL character is accounted for
> > in
> > DM_NAME_LEN.
>
> Then it was broken before.
>
> sizeof("1234") == strlen("1234") + 1 == 5
Hmmm...wasn't aware of that. I guess I never expected there to be a
difference. If that's the case, then Joe Perches' earlier patch should do the
trick, albiet for obscure reasons.
And I wouldn't say it was broken the other way; it was simply allocating one
byte more than necessary.
--
Kevin Corry
[email protected]
http://evms.sourceforge.net/