Re: [PATCH] scsi_debug: implement 'sdebug_lun_format' and update max_lun

[Bart Van Assche <bvanassche@acm.org>]

On 2020-05-29 06:39, Hannes Reinecke wrote:
> -/* If REPORT LUNS has luns >= 256 it can choose "flat space" (value 1)
> - * or "peripheral device" addressing (value 0) */
> +/* If REPORT LUNS has luns >= 256 it should choose "flat space" (value 1)
> + * instead of "peripheral device" addressing (value 0) */
>  #define SAM2_LUN_ADDRESS_METHOD 0

How about introducing an enumeration type instead of using #define?

>  /* SDEBUG_CANQUEUE is the maximum number of commands that can be queued
> @@ -666,6 +666,7 @@ static bool have_dif_prot;
>  static bool write_since_sync;
>  static bool sdebug_statistics = DEF_STATISTICS;
>  static bool sdebug_wp;
> +static int sdebug_lun_format = SAM2_LUN_ADDRESS_METHOD;

How about using an enumeration type instead of 'int'?

> +static ssize_t lun_format_show(struct device_driver *ddp, char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", sdebug_lun_format);
> +}

How about inserting a blank line after this function definition?

> +static ssize_t lun_format_store(struct device_driver *ddp, const char *buf,
> +				size_t count)
> +{
> +	int n;
> +	bool changed;
> +
> +	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n >= 0)) {

Isn't the recommended coding style to place constants at the right side
of comparisons? How about leaving out the superfluous parentheses? Such
parentheses may hide bugs because these suppress compiler warnings about
using assignments in if-conditions.

> +		if (n > 2) {
> +			pr_warn("only formats 0 and 1 are supported\n");
> +			return -EINVAL;
> +		}

How about introducing a symbolic name for the constant '2'?


> +	if (sdebug_lun_format > 2) {
> +		pr_warn("Invalid LUN format %u, using default\n",
> +			sdebug_lun_format);
> +		sdebug_lun_format = SAM2_LUN_ADDRESS_METHOD;
> +	}

How about using a symbolic name instead of the constant '2'?

>  	if (sdebug_max_luns > 256) {
> -		pr_warn("max_luns can be no more than 256, use default\n");
> -		sdebug_max_luns = DEF_MAX_LUNS;
> +		if (sdebug_max_luns > 16384) {
> +			pr_warn("max_luns can be no more than 16384, use default\n");
> +			sdebug_max_luns = DEF_MAX_LUNS;
> +		}
> +		sdebug_lun_format = 1;
>  	}

How about changing the constant '1' into a symbolic name?

Thanks,

Bart.