scsi: ses check name in enclosure_component_register

PATCH In review Yinghai Lu 3 participants 9 messages Apr 28, 2009 – May 1, 2009

Subsystems (2)

SCSI Char and misc drivers

lore

Share & tools

lore

Yinghai Lu

author

2009-04-28 02:19 UTC #0 lore raw

PATCH 1/1 scsi: ses check name in enclosure_component_register

dev_set_name will use sprintf to copy the name.
need to check if the name does valid.

otherwise will error from device_add later.

Signed-off-by Yinghai Lu <[email protected]>

drivers/misc/enclosure.c +1 -1

index 3cf61ec..68743a9 100644
@@ -255,7 +255,7 @@ enclosure_component_register(struct enclosure_device *edev,
 	ecomp->number = number;
 	cdev = &ecomp->cdev;
 	cdev->parent = get_device(&edev->edev);
-	if (name)
+	if (name && name[0])
 		dev_set_name(cdev, name);
 	else
 		dev_set_name(cdev, "%u", number);

James Bottomley

maintainer

2009-04-28 22:26 UTC #1 lore raw

Quoted 4 lines · Yinghai Lu

dev_set_name will use sprintf to copy the name.
need to check if the name does valid.
otherwise will error from device_add later.

I think what you mean to say is that empty names aren't allowed in the device model.

drivers/misc/enclosure.c enclosure_component_register(struct enclosure_device *edev, · line 259

Quoted 13 lines

Signed-off-by Yinghai Lu <[email protected]>

drivers/misc/enclosure.c +1 -1

index 3cf61ec..68743a9 100644
@@ -255,7 +255,7 @@ enclosure_component_register(struct enclosure_device *edev,
 	ecomp->number = number;
 	cdev = &ecomp->cdev;
 	cdev->parent = get_device(&edev->edev);
-	if (name)
+	if (name && name[0])
 		dev_set_name(cdev, name);

Actually, I think this should become

dev_set_name(cdev, "%s", name);

as well, otherwise any name with a percent in it will get interpreted in ways we're not expecting at all.

James

Yinghai Lu

author

2009-04-28 23:57 UTC #2 lore raw

Quoted 25 lines · James Bottomley

Quoted 4 lines · Yinghai Lu
dev_set_name will use sprintf to copy the name.
need to check if the name does valid.
otherwise will error from device_add later.
I think what you mean to say is that empty names aren't allowed in the device model.
drivers/misc/enclosure.c enclosure_component_register(struct enclosure_device *edev, · line 259
Quoted 13 lines
Signed-off-by Yinghai Lu <[email protected]>
drivers/misc/enclosure.c +1 -1
index 3cf61ec..68743a9 100644
@@ -255,7 +255,7 @@ enclosure_component_register(struct enclosure_device *edev,
 	ecomp->number = number;
 	cdev = &ecomp->cdev;
 	cdev->parent = get_device(&edev->edev);
-	if (name)
+	if (name && name[0])
 		dev_set_name(cdev, name);
Actually, I think this should become

Quoted 4 lines

dev_set_name(cdev, "%s", name);
as well, otherwise any name with a percent in it will get interpreted in ways we're not expecting at all.

i tried that it still failed.

James Bottomley

maintainer

2009-04-29 00:18 UTC #3 lore raw

Quoted 26 lines · Yinghai Lu

Quoted 25 lines · James Bottomley
Quoted 4 lines · Yinghai Lu
dev_set_name will use sprintf to copy the name.
need to check if the name does valid.
otherwise will error from device_add later.
I think what you mean to say is that empty names aren't allowed in the device model.
drivers/misc/enclosure.c enclosure_component_register(struct enclosure_device *edev, · line 259
Quoted 13 lines
Signed-off-by Yinghai Lu <[email protected]>
drivers/misc/enclosure.c +1 -1
index 3cf61ec..68743a9 100644
@@ -255,7 +255,7 @@ enclosure_component_register(struct enclosure_device *edev,
 	ecomp->number = number;
 	cdev = &ecomp->cdev;
 	cdev->parent = get_device(&edev->edev);
-	if (name)
+	if (name && name[0])
 		dev_set_name(cdev, name);
Actually, I think this should become

Quoted 6 lines

Quoted 4 lines
dev_set_name(cdev, "%s", name);
as well, otherwise any name with a percent in it will get interpreted in ways we're not expecting at all.
i tried that it still failed.

I didn't say it was the fix to your problem. I said on looking at the code it's another potential problem source. Your problem is that filesystems don't support empty file names.

James

Yinghai Lu

author

2009-04-29 01:03 UTC #4 lore raw

Quoted 27 lines · James Bottomley

Quoted 26 lines · Yinghai Lu
Quoted 25 lines · James Bottomley
Quoted 4 lines · Yinghai Lu
dev_set_name will use sprintf to copy the name.
need to check if the name does valid.
otherwise will error from device_add later.
I think what you mean to say is that empty names aren't allowed in the device model.
drivers/misc/enclosure.c enclosure_component_register(struct enclosure_device *edev, · line 259
Quoted 13 lines
Signed-off-by Yinghai Lu <[email protected]>
drivers/misc/enclosure.c +1 -1
index 3cf61ec..68743a9 100644
@@ -255,7 +255,7 @@ enclosure_component_register(struct enclosure_device *edev,
 	ecomp->number = number;
 	cdev = &ecomp->cdev;
 	cdev->parent = get_device(&edev->edev);
-	if (name)
+	if (name && name[0])
 		dev_set_name(cdev, name);
Actually, I think this should become
dev_set_name(cdev, "%s", name);

Quoted 7 lines

Quoted 3 lines
Quoted 2 lines
as well, otherwise any name with a percent in it will get interpreted in ways we're not expecting at all.
i tried that it still failed.
I didn't say it was the fix to your problem. I said on looking at the code it's another potential problem source. Your problem is that filesystems don't support empty file names.

dev_set_name(cdev, name);

the name become fmt.

Matthew Wilcox

2009-04-29 02:03 UTC #5 lore raw

Quoted 3 lines · Yinghai Lu

dev_set_name(cdev, name);
the name become fmt.

Yes, and what happens if 'name' is '%s'?

That's James's point.

Signature · 4 lines

Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Yinghai Lu

author

2009-04-29 03:35 UTC #6 lore raw

Quoted 7 lines · Matthew Wilcox

Quoted 3 lines · Yinghai Lu
dev_set_name(cdev, name);
the name become fmt.
Yes, and what happens if 'name' is '%s'?

change
dev_set_name(cdev, name);
to
dev_set_name(cdev, "%s", name);

didn't fix the problem.

need to check name && name[0]

Matthew Wilcox

2009-04-29 11:08 UTC #7 lore raw

Quoted 9 lines · Yinghai Lu

Quoted 7 lines · Matthew Wilcox
Quoted 3 lines · Yinghai Lu
dev_set_name(cdev, name);
the name become fmt.
Yes, and what happens if 'name' is '%s'?

Quoted 8 lines

change
dev_set_name(cdev, name);
to
dev_set_name(cdev, "%s", name);
didn't fix the problem.
need to check name && name[0]

Yes. There are _two_ problems. You fixed one. James asked if you would fix the other while you're touching that code.

Signature · 4 lines

Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Yinghai Lu

author

2009-05-01 02:14 UTC #8 lore raw

PATCH 1/1 scsi: ses check name in enclosure_component_register -v2

dev_set_name will use sprintf to copy the name.
need to check if the name does valid.

otherwise will error from device_add later.

v2: add %s in dev_set_name according to James

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/misc/enclosure.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/misc/enclosure.c +3 -3

Index: linux-2.6/drivers/misc/enclosure.c
===================================================================
@@ -119,7 +119,7 @@ enclosure_register(struct device *dev, c
 	edev->edev.class = &enclosure_class;
 	edev->edev.parent = get_device(dev);
 	edev->cb = cb;
-	dev_set_name(&edev->edev, name);
+	dev_set_name(&edev->edev, "%s", name);
 	err = device_register(&edev->edev);
 	if (err)
 		goto err;

@@ -255,8 +255,8 @@ enclosure_component_register(struct encl
 	ecomp->number = number;
 	cdev = &ecomp->cdev;
 	cdev->parent = get_device(&edev->edev);
-	if (name)
-		dev_set_name(cdev, name);
+	if (name && name[0])
+		dev_set_name(cdev, "%s", name);
 	else
 		dev_set_name(cdev, "%u", number);