On 06/18/2019 08:26 AM, Bart Van Assche wrote:
> On 6/17/19 10:42 PM, Chaitanya Kulkarni wrote:
>> +inline const char *blk_op_str(int op)
>> +{
>> + const char *op_str = "UNKNOWN";
>> +
>> + if (op < ARRAY_SIZE(blk_op_name) && blk_op_name[op])
>> + op_str = blk_op_name[op];
>> +
>> + return op_str;
>> +}
>
> This won't work correctly if op < 0. If you have another look at the
> block layer debugfs code you will see that 'op' is has an unsigned type
> in that code. Please either change the type of 'op' from 'int' to
> 'unsigned int' or change 'op < ARRAY_SIZE(blk_op_name)' into
> '(unsigned)op < ARRAY_SIZE(blk_op_name)'.
>
Sorry I'm little confused here, even though op is defined as a unsigned
int we print it as a "%d". So I think we need to keep that as it is or
I'll be happy to send a corrective patch to print it is using %u, your
input will be very useful here.
Regarding the correctness, I think if one of the operand is unsigned
then signed values are converted to unsigned valued implicitly.
However the exceptions is if the type of the operand with signed integer
type can represent all of the values of the type of the operand with
unsigned integer type, then the operand with unsigned integer type is
converted to the type of the operand with signed integer type.
I'm wondering how would above scenario occur when comparing int and
size_t. (unless on a platform int can fit all the values into size_t).
Since above comparison of the ARRAY_SIZE involves sizeof (size_t) type
is a base unsigned integer value, even if op < 0 it will get converted
into the unsigned and it will still work.
Please correct me if I'm wrong.
In order to validate my claim I ran a test with simple test module
here are the result:-
void test(void)
{
pr_info("%s\n", blk_op_str(REQ_OP_READ));
pr_info("%s\n", blk_op_str(REQ_OP_WRITE));
pr_info("%s\n", blk_op_str(REQ_OP_FLUSH));
pr_info("%s\n", blk_op_str(REQ_OP_DISCARD));
pr_info("%s\n", blk_op_str(REQ_OP_SECURE_ERASE));
pr_info("%s\n", blk_op_str(REQ_OP_ZONE_RESET));
pr_info("%s\n", blk_op_str(REQ_OP_WRITE_SAME));
pr_info("%s\n", blk_op_str(REQ_OP_WRITE_ZEROES));
pr_info("%s\n", blk_op_str(REQ_OP_SCSI_IN));
pr_info("%s\n", blk_op_str(REQ_OP_SCSI_OUT));
pr_info("%s\n", blk_op_str(REQ_OP_DRV_IN));
pr_info("%s\n", blk_op_str(REQ_OP_DRV_OUT));
pr_info("LAST %s\n", blk_op_str(REQ_OP_LAST));
pr_info("LAST + 1 %s\n", blk_op_str(REQ_OP_LAST + 1));
pr_info("-1 %s\n", blk_op_str(-1));
pr_info("-2 %s\n", blk_op_str(-2));
pr_info("-3 %s\n", blk_op_str(-3));
pr_info("-4 %s\n", blk_op_str(-4));
}
[ 819.336023] READ
[ 819.336030] WRITE
[ 819.336034] FLUSH
[ 819.336037] DISCARD
[ 819.336041] SECURE_ERASE
[ 819.336044] ZONE_RESET
[ 819.336048] WRITE_SAME
[ 819.336051] WRITE_ZEROES
[ 819.336054] SCSI_IN
[ 819.336058] SCSI_OUT
[ 819.336061] DRV_IN
[ 819.336064] DRV_OUT
[ 819.336068] LAST UNKNOWN
[ 819.336071] LAST + 1 UNKNOWN
[ 819.336075] -1 UNKNOWN
[ 819.336078] -2 UNKNOWN
[ 819.336081] -3 UNKNOWN
[ 819.336084] -4 UNKNOWN
I'll make this change op int->unsigned as it is present in the debugfs
code so that it will be consistent, thanks again for looking into this.
> Thanks,
>
> Bart.
>
