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

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

[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 <keescook@chromium.org>
Reviewed-by: Scott Teel <scott.teel@pmcs.com>
Reviewed-by: Webb Scales <webbnh@hp.com>
Signed-off-by: Don Brace <don.brace@pmcs.com>
---
 drivers/scsi/hpsa.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 95d581c..a1cfbd3 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -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);
 

Re: [PATCH] hpsa: correct compiler warnings introduced by hpsa-add-local-workqueue patch

[James Bottomley]

On Fri, 2015-02-06 at 17:44 -0600, Don Brace wrote:
> Correct compiler warning introduced by hpsa-add-local-workqueue patch
> 6636e7f455b33b957c5ee016daa6de46148026ab hpsa: Use local workqueues
> instead of system workqueues
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Scott Teel <scott.teel@pmcs.com>
> Reviewed-by: Webb Scales <webbnh@hp.com>
> Signed-off-by: Don Brace <don.brace@pmcs.com>
> ---
>  drivers/scsi/hpsa.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 95d581c..a1cfbd3 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -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

Re: [PATCH] hpsa: correct compiler warnings introduced by hpsa-add-local-workqueue patch

[Tejun Heo]

On Tue, Feb 10, 2015 at 08:56:30AM -0800, James Bottomley wrote:
> > +	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.

-- 
tejun