Re: [PATCH v2] scsi_debug: randomize command duration option + %p

["Martin K. Petersen" <martin.petersen@oracle.com>]

Doug,

>> Since sdebug_random is either 0 or 1, is the "? 1 : 0" part necessary?
>
> No.
> Why didn't you complain when MKP wrote that? That is where I cut and
> pasted that code from (sdebug_removable).

[...]

> Again, copy and paste from MKP's code (which no doubt was a copy +
> paste from my earlier code).

I'll be the first to admit that I make mistakes and almost always
blindly copy surrounding plumbing when I hack on scsi_debug. However, I
am not responsible for the commit that introduced any of this code.

Also, in defense of the original author, our coding practices were
obviously different when this was written many years ago.

>>> @@ -5338,7 +5373,7 @@ static int __init scsi_debug_init(void)
>>>           dif_size = sdebug_store_sectors * sizeof(struct t10_pi_tuple);
>>>           dif_storep = vmalloc(dif_size);
>>> -        pr_err("dif_storep %u bytes @ %p\n", dif_size, dif_storep);
>>> +        pr_err("dif_storep %u bytes @ %pK\n", dif_size, dif_storep);
>>>           if (dif_storep == NULL) {
>>>               pr_err("out of mem. (DIX)\n");
>>
>> Is it useful to print the kernel pointer 'dif_storep'?
>
> Ask MKP, it's his code. All I do know is that doing a printk("%p" ...)
> is useless in lk 5.3 (and probably lk 5.2). Taking the above snippet,
> if vmalloc() returned NULL then the existing pr_err() would print out
> a random number rather than <null>. Sick ...

This used to be somewhat useful for debugging purposes in combination
with the ability to dump the buffer to inspect the PI. In this day and
age with obfuscated kernel pointers, not so much. I suggest just
removing the line. Or--if people feel the size is valuable
information--just zap the pointer. Also, since this is unrelated to the
duration randomization it should be a separate patch.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering