Re: [PATCH 04/24] scsi: introduce sdev_prefix_printk()

[Hannes Reinecke <hare@suse.de>]

On 10/03/2014 06:31 PM, Ewan Milne wrote:
> On Thu, 2014-10-02 at 18:37 +0000, Elliott, Robert (Server Storage)
> wrote:
>>> -----Original Message-----
>>> From: Hannes Reinecke [mailto:hare@suse.de]
>> ...
>>> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
>>> index 4c3ab83..c01dc89 100644
>>> --- a/drivers/scsi/sd.h
>>> +++ b/drivers/scsi/sd.h
>>> @@ -103,9 +103,10 @@ static inline struct scsi_disk *scsi_disk(struct gendisk
>>> *disk)
>>>
>>>  #define sd_printk(prefix, sdsk, fmt, a...)				\
>>>          (sdsk)->disk ?							\
>>> -	sdev_printk(prefix, (sdsk)->device, "[%s] " fmt,		\
>>> -		    (sdsk)->disk->disk_name, ##a) :			\
>>> -	sdev_printk(prefix, (sdsk)->device, fmt, ##a)
>>> +	      sdev_prefix_printk(prefix, (sdsk)->device,		\
>>> +				 (sdsk)->disk->disk_name, fmt, ##a) :	\
>>> +	      sdev_prefix_printk(prefix, (sdsk)->device,		\
>>> +				 NULL, fmt, ##a)
>>>
>>>  #define sd_first_printk(prefix, sdsk, fmt, a...)			\
>>>  	do {								\
>> ...
>>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>>> index 27ecee7..0b18a09 100644
>>> --- a/include/scsi/scsi_device.h
>>> +++ b/include/scsi/scsi_device.h
>>> @@ -244,6 +244,15 @@ struct scsi_dh_data {
>>>  #define sdev_dbg(sdev, fmt, a...) \
>>>  	dev_dbg(&(sdev)->sdev_gendev, fmt, ##a)
>>>
>>> +/*
>>> + * like scmd_printk, but the device name is passed in
>>> + * as a string pointer
>>> + */
>>> +#define sdev_prefix_printk(l, sdev, p, fmt, a...)			\
>>> +	(p) ?								\
>>> +	sdev_printk(l, sdev, "[%s] " fmt, p, ##a) :			\
>>> +	sdev_printk(l, sdev, fmt, ##a)
>>> +
>>>  #define scmd_printk(prefix, scmd, fmt, a...)				\
>>>          (scmd)->request->rq_disk ?					\
>>>  	sdev_printk(prefix, (scmd)->device, "[%s] " fmt,		\
>>> --
>>> 1.8.5.2
>>
>> This triggers lots of compiler warnings with gcc 4.4.7 like:
>>
>> drivers/scsi/sd.c: In function 'sd_open':
>> drivers/scsi/sd.c:1179: warning: reading through null pointer (argument 4)
>> drivers/scsi/sd.c:1179: warning: format '%s' expects type 'char *', but argument 4 has type 'void *'
>>
>>
>> That is from:
>>         SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp, "sd_open\n"));
>>
>> Since:
>> #define NULL ((void *)0)
>>
>> gcc probably doesn't realize the (p)? prevents the NULL (a void *) 
>> from being passed to sdev_printk.
>>
>> Passing "" rather than NULL eliminates the compiler warnings.
> 
> It eliminates the warnings, but unfortunately we then get log messages
> that look like:
> 
> Oct  3 11:30:08 rhel-storage-01 kernel: sd 10:0:0:0: [sde] (null)FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> 
>                                                            ^^^^^^
> 
> Changing it to (char *)NULL, like this:
> 
> #define sd_printk(prefix, sdsk, fmt, a...)                              \
>         (sdsk)->disk ?                                                  \
>               sdev_prefix_printk(prefix, (sdsk)->device,                \
>                                  (sdsk)->disk->disk_name, fmt, ##a) :   \
>               sdev_prefix_printk(prefix, (sdsk)->device,                \
>                                  (char *)NULL, fmt, ##a)
> 
> doesn't work either.  The compiler gives an error:
> 
> drivers/scsi/sd.c: In function 'sd_open':
> drivers/scsi/sd.c:1158:2: error: reading through null pointer (argument 4) [-Werror=format=]
>   SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp, "sd_open\n"));
>   ^
> 

I've fixed it up in my next iteration of the patchset.
(By simply using 'sdev_printk' if the prefix argument is NULL ...)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html