Skip to contentSkip to conversation

hpsa: correct compiler warnings introduced by hpsa-add-local-workqueue patch

PATCH In review Don Brace 3 participants 3 messages Feb 6, 2015 – Feb 10, 2015

Subsystems (2)
SCSI Hewlett-packard smart array RAID driver (hpsa)
lore
Share & tools
lore

Don Brace

author
#0 lore raw

PATCH 1/1 hpsa: correct compiler warnings introduced by hpsa-add-local-workqueue patch

Correct compiler warning introduced by hpsa-add-local-workqueue patch 6636e7f455b33b957c5ee016daa6de46148026ab hpsa: Use local workqueues instead of system workqueues

Suggested-by Kees Cook <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Reviewed-by Webb Scales <[email protected]>
Signed-off-by Don Brace <[email protected]>
---
 drivers/scsi/hpsa.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/scsi/hpsa.c +1 -3
index 95d581c..a1cfbd3 100644
@@ -6831,10 +6831,8 @@ static struct workqueue_struct *hpsa_create_controller_wq(struct ctlr_info *h,
 						char *name)
 {
 	struct workqueue_struct *wq = NULL;
-	char wq_name[20];
 
-	snprintf(wq_name, sizeof(wq_name), "%s_%d_hpsa", name, h->ctlr);
-	wq = alloc_ordered_workqueue(wq_name, 0);
+	wq = alloc_ordered_workqueue("%s_%d_hpsa", 0, name, h->ctlr);
 	if (!wq)
 		dev_err(&h->pdev->dev, "failed to create %s workqueue\n", name);
maintainer
#1 lore raw
Quoted 17 lines · Don Brace

Correct compiler warning introduced by hpsa-add-local-workqueue patch 6636e7f455b33b957c5ee016daa6de46148026ab hpsa: Use local workqueues instead of system workqueues

Suggested-by Kees Cook <[email protected]>
Reviewed-by Scott Teel <[email protected]>
Reviewed-by Webb Scales <[email protected]>
Signed-off-by Don Brace <[email protected]>
---
 drivers/scsi/hpsa.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/scsi/hpsa.c +0 -0
index 95d581c..a1cfbd3 100644

drivers/scsi/hpsa.c static struct workqueue_struct *hpsa_create_controller_wq(struct ctlr_info *h, · line 6835

Quoted 8 lines
drivers/scsi/hpsa.c +1 -3
@@ -6831,10 +6831,8 @@ static struct workqueue_struct *hpsa_create_controller_wq(struct ctlr_info *h,
 						char *name)
 {
 	struct workqueue_struct *wq = NULL;
-	char wq_name[20];
 
-	snprintf(wq_name, sizeof(wq_name), "%s_%d_hpsa", name, h->ctlr);
-	wq = alloc_ordered_workqueue(wq_name, 0);
+	wq = alloc_ordered_workqueue("%s_%d_hpsa", 0, name, h->ctlr);

It's not an objection to your patch, but what idiot did this? There's an extra variable there between the format and the arguments. That makes the pattern counterintuitive (i.e. an interface easy to get wrong) because everywhere else, the arguments immediately follow the format argument. Please never, ever do this again.

By the way, the above is rhetorical ... the culprit is in the cc.

James

#2 lore raw
Quoted 9 lines · James Bottomley

drivers/scsi/hpsa.c static struct workqueue_struct *hpsa_create_controller_wq(struct ctlr_info *h, · line 6835

Quoted 1 line
drivers/scsi/hpsa.c +1 -0
+	wq = alloc_ordered_workqueue("%s_%d_hpsa", 0, name, h->ctlr);

It's not an objection to your patch, but what idiot did this? There's an extra variable there between the format and the arguments. That makes the pattern counterintuitive (i.e. an interface easy to get wrong) because everywhere else, the arguments immediately follow the format argument. Please never, ever do this again.

By the way, the above is rhetorical ... the culprit is in the cc.

LOL, that idiot would be me. Well, it has a history. Those functions originally only took static names and only way later got augmented to take printf format and, at that point, only few needed formatting. I can argue both sides of changing the parameter order. It's one of the easiest mistakes to spot after all.

Thanks.

Signature · 1 line
tejun