Re: [ndctl PATCH v2 1/2] daxctl: Fix create-device parameters parsing

["Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com>]

On 01/06/2024 05:32, Alison Schofield wrote:
> On Fri, May 31, 2024 at 02:29:58PM +0800, Li Zhijian wrote:
>> Previously, the extra parameters will be ignored quietly, which is a bit
>> weird and confusing.
> 
> It's just wrong. There is code to catch extra params, but it's being
> skipped because of the index setting that you mention below. Suggest
> referencing the incorrect index is causing the extra params to be
> ignored.
> 
> Suggest commit msg of:
> daxctl: Fail create-device if extra parameters are present

Sounds good to me,

Will fix it and other below suggestions.


Thanks
Zhijian


> 
> 
>> $ daxctl create-device region0
>> [
>>    {
>>      "chardev":"dax0.1",
>>      "size":268435456,
>>      "target_node":1,
>>      "align":2097152,
>>      "mode":"devdax"
>>    }
>> ]
>> created 1 device
>>
>> where above user would want to specify '-r region0'.
>>
>> Check extra parameters starting from index 0 to ensure no extra parameters
>> are specified for create-device.
>>
>> Cc: Fan Ni <fan.ni@samsung.com>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>> V2:
>> Remove the external link[0] in case it get disappeared in the future.
>> [0] https://github.com/moking/moking.github.io/wiki/cxl%E2%80%90test%E2%80%90tool:-A-tool-to-ease-CXL-test-with-QEMU-setup%E2%80%90%E2%80%90Using-DCD-test-as-an-example#convert-dcd-memory-to-system-ram
>> ---
>>   daxctl/device.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/daxctl/device.c b/daxctl/device.c
>> index 839134301409..ffabd6cf5707 100644
>> --- a/daxctl/device.c
>> +++ b/daxctl/device.c
>> @@ -363,7 +363,8 @@ static const char *parse_device_options(int argc, const char **argv,
>>   		NULL
>>   	};
>>   	unsigned long long units = 1;
>> -	int i, rc = 0;
>> +	int rc = 0;
>> +	int i = action == ACTION_CREATE ? 0 : 1;
> 
> This confuses me because at this point I don't know what 'i' will be
> used for.  How about moving the setting near the usage below -
> 
>>   	char *device = NULL;
>>   
>>   	argc = parse_options(argc, argv, options, u, 0);
>> @@ -402,7 +403,7 @@ static const char *parse_device_options(int argc, const char **argv,
>>   			action_string);
>>   		rc = -EINVAL;
>>   	}
>> -	for (i = 1; i < argc; i++) {
>> +	for (; i < argc; i++) {
>>   		fprintf(stderr, "unknown extra parameter \"%s\"\n", argv[i]);
>>   		rc = -EINVAL;
>>   	}
> 
> Something like this:
> 
> diff --git a/daxctl/device.c b/daxctl/device.c
> index 14d62148c58a..6c0758101c4a 100644
> --- a/daxctl/device.c
> +++ b/daxctl/device.c
> @@ -402,6 +402,8 @@ static const char *parse_device_options(int argc, const char **argv,
>                          action_string);
>                  rc = -EINVAL;
>          }
> +       /* ACTION_CREATE expects 0 parameters */
> +       i = action == ACTION_CREATE ? 0 : 1;
>          for (i = 1; i < argc; i++) {
>                  fprintf(stderr, "unknown extra parameter \"%s\"\n", argv[i]);
>                  rc = -EINVAL;
> 
> 
> 
> 
> 
> 
>> -- 
>> 2.29.2
>>
>>