Re: [PATCH v3 03/20] sg: sg_log and is_enabled

[Douglas Gilbert <dgilbert@interlog.com>]

On 2019-08-12 10:23 a.m., Christoph Hellwig wrote:
> On Wed, Aug 07, 2019 at 01:42:35PM +0200, Douglas Gilbert wrote:
>> Replace SCSI_LOG_TIMEOUT macros with SG_LOG macros across the driver.
>> The definition of SG_LOG calls SCSI_LOG_TIMEOUT if given and derived
>> pointers are non-zero, calls pr_info otherwise. SG_LOGS additionally
>> prints the sg device name and the thread id. The thread id is very
>> useful, even in single threaded invocations because the driver not
>> only uses the invocer's thread but also uses work queues and the
>> main callback (i.e. sg_rq_end_io()) may hit any thread. Some
>> interesting cases arise when the callback hits its invocer's
>> thread.
>>
>> SG_LOGS takes 48 bytes on the stack to build this printf format
>> string: "sg%u: tid=%d" whose size is clearly bounded above by
>> the maximum size of those two integers.
>> Protecting against the 'current' pointer being zero is for safety
>> and the case where the boot device is SCSI and the sg driver is
>> built into the kernel. Also when debugging, getting a message
>> from a compromised kernel can be very useful in pinpointing the
>> location of the failure.
>>
>> The simple fact that the SG_LOG macro is shorter than
>> SCSI_LOG_TIMEOUT macro allow more error message "payload" per line.
>>
>> Also replace #if and #ifdef conditional compilations with
>> the IS_ENABLED macro.
>>
>> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
>> ---
>>   drivers/scsi/sg.c | 252 +++++++++++++++++++++++-----------------------
>>   1 file changed, 125 insertions(+), 127 deletions(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index 6615777931f7..d14ba4a5441c 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -57,6 +57,15 @@ static char *sg_version_date = "20190606";
>>   
>>   #define SG_MAX_DEVS 32768
>>   
>> +/* Comment out the following line to compile out SCSI_LOGGING stuff */
>> +#define SG_DEBUG 1
>> +
>> +#if !IS_ENABLED(SG_DEBUG)
>> +#if IS_ENABLED(DEBUG)	/* If SG_DEBUG not defined, check for DEBUG */
>> +#define SG_DEBUG DEBUG
>> +#endif
>> +#endif
> 
> IS_ENABLED is mostly useful for checking it from C-level if statements.
> No need to use this from cpp.  But even more importantly we generally
> try to avoid cpp checks that aren't driven from Kconfig.  Please make
> these an actual CONFIG_ options.

Another reviewer (called James) didn't like #ifdef_s (at file scope).

Surely it is simpler to allow an experienced C programmer to add

#define DEBUG 1
   or
#define SG_DEBUG 1

to the driver source code than to add to the existing Linux config
nightmare with a new config parameter:
   CONFIG_CHR_DEV_SG_DEBUG

$ grep "^CONF" <linux-stable>/.config | wc
2480

on my system.
Linux: where too much is never enough.

>>   static int sg_read_oxfer(struct sg_request *srp, char __user *outp,
>> -			 int num_read_xfer);
>> +			 int num_xfer);
> 
> This looks like a random unrelated change.

Separate patch maybe :-)

>> -#define SZ_SG_HEADER sizeof(struct sg_header)
>> -#define SZ_SG_IO_HDR sizeof(sg_io_hdr_t)
>> -#define SZ_SG_IOVEC sizeof(sg_iovec_t)
>> -#define SZ_SG_REQ_INFO sizeof(sg_req_info_t)
>> +#define SZ_SG_HEADER ((int)sizeof(struct sg_header))	/* v1 and v2 header */
>> +#define SZ_SG_IO_HDR ((int)sizeof(struct sg_io_hdr))	/* v3 header */
>> +#define SZ_SG_REQ_INFO ((int)sizeof(struct sg_req_info))
> 
> Doesn't look related to the patch.  But more importantly there should
> be no point to cast or even have the macros wrapping the sizeof to
> start with.

Well I find it useful as I spend a fair bit of time re-reading
my code. To me a dozen upper case letters is easier to decode
than a sea of parentheses.

And the tide is moving back against "unsigned" types, at least in
C++. It is now regarded as a mistake the way STL introduced lots
of "unsigned" types and they are looking at whether they can move
some of them back to int.
Closer to home, I use ints wherever possible and the kernel seems
to favour them with negated errno return values. However the compiler
mindlessly warns about signed/unsigned comparisons when an int is
compared to a sizeof(). [If it had half a brain, it would check the
_value_ of the sizeof, and realize that often, even on an 8 bit
machine, there is no reason for the warning.]

Then there is the "Fortran rule" in checkpatch.pl (i.e. no lines
than 80 characters). "(int)sizeof(struct sg_io_hdr)" is 30 characters
long and that length often pushes simple conditionals like:
     if (A > B)
onto two lines which IMO is stupid.

Doug Gilbert