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

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

[Li Zhijian]

Previously, the extra parameters will be ignored quietly, which is a bit
weird and confusing.
$ 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;
 	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;
 	}
-- 
2.29.2

[ndctl PATCH v2 2/2] daxctl: Remove unimplemented create-device options

[Li Zhijian]

RECONFIG_OPTIONS and ZONE_OPTIONS are not implemented for create-device
and they will be ignored by create-device. Remove them so that the usage
message is identical to the manual.

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
V2: make the usage match the manual because the usage is wrong.
---
 daxctl/device.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/daxctl/device.c b/daxctl/device.c
index ffabd6cf5707..781dc4007f83 100644
--- a/daxctl/device.c
+++ b/daxctl/device.c
@@ -98,8 +98,6 @@ OPT_BOOLEAN('\0', "no-movable", &param.no_movable, \
 static const struct option create_options[] = {
 	BASE_OPTIONS(),
 	CREATE_OPTIONS(),
-	RECONFIG_OPTIONS(),
-	ZONE_OPTIONS(),
 	OPT_END(),
 };
 
-- 
2.29.2

Re: [ndctl PATCH v2 2/2] daxctl: Remove unimplemented create-device options

[Verma, Vishal L]

On Fri, 2024-05-31 at 14:29 +0800, Li Zhijian wrote:
> RECONFIG_OPTIONS and ZONE_OPTIONS are not implemented for create-
> device
> and they will be ignored by create-device. Remove them so that the
> usage
> message is identical to the manual.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> ---
> V2: make the usage match the manual because the usage is wrong.
> ---
>  daxctl/device.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/daxctl/device.c b/daxctl/device.c
> index ffabd6cf5707..781dc4007f83 100644
> --- a/daxctl/device.c
> +++ b/daxctl/device.c
> @@ -98,8 +98,6 @@ OPT_BOOLEAN('\0', "no-movable", &param.no_movable,
> \
>  static const struct option create_options[] = {
>  	BASE_OPTIONS(),
>  	CREATE_OPTIONS(),
> -	RECONFIG_OPTIONS(),
> -	ZONE_OPTIONS(),
>  	OPT_END(),
>  };
>  

Re: [ndctl PATCH v2 2/2] daxctl: Remove unimplemented create-device options

[Dave Jiang]

On 5/30/24 11:29 PM, Li Zhijian wrote:
> RECONFIG_OPTIONS and ZONE_OPTIONS are not implemented for create-device
> and they will be ignored by create-device. Remove them so that the usage
> message is identical to the manual.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> V2: make the usage match the manual because the usage is wrong.
> ---
>  daxctl/device.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/daxctl/device.c b/daxctl/device.c
> index ffabd6cf5707..781dc4007f83 100644
> --- a/daxctl/device.c
> +++ b/daxctl/device.c
> @@ -98,8 +98,6 @@ OPT_BOOLEAN('\0', "no-movable", &param.no_movable, \
>  static const struct option create_options[] = {
>  	BASE_OPTIONS(),
>  	CREATE_OPTIONS(),
> -	RECONFIG_OPTIONS(),
> -	ZONE_OPTIONS(),
>  	OPT_END(),
>  };
>  

Re: [ndctl PATCH v2 2/2] daxctl: Remove unimplemented create-device options

[Alison Schofield]

On Fri, May 31, 2024 at 02:29:59PM +0800, Li Zhijian wrote:
> RECONFIG_OPTIONS and ZONE_OPTIONS are not implemented for create-device
> and they will be ignored by create-device. Remove them so that the usage
> message is identical to the manual.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

The net effect of this is a correction to the usage message.
I think that can fit in the commit msg with something like this:

daxctl: Remove unused options in create-device usage message

I'm not familiar with this style of patch 2 being a reply to patch 1.
Is there a reason this is not presented as a patchset?

Thanks,
Alison


> ---
> V2: make the usage match the manual because the usage is wrong.
> ---
>  daxctl/device.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/daxctl/device.c b/daxctl/device.c
> index ffabd6cf5707..781dc4007f83 100644
> --- a/daxctl/device.c
> +++ b/daxctl/device.c
> @@ -98,8 +98,6 @@ OPT_BOOLEAN('\0', "no-movable", &param.no_movable, \
>  static const struct option create_options[] = {
>  	BASE_OPTIONS(),
>  	CREATE_OPTIONS(),
> -	RECONFIG_OPTIONS(),
> -	ZONE_OPTIONS(),
>  	OPT_END(),
>  };
>  
> -- 
> 2.29.2
> 
> 

Re: [ndctl PATCH v2 2/2] daxctl: Remove unimplemented create-device options

[Zhijian Li (Fujitsu)]

On 01/06/2024 05:43, Alison Schofield wrote:
> On Fri, May 31, 2024 at 02:29:59PM +0800, Li Zhijian wrote:
>> RECONFIG_OPTIONS and ZONE_OPTIONS are not implemented for create-device
>> and they will be ignored by create-device. Remove them so that the usage
>> message is identical to the manual.
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> 
> The net effect of this is a correction to the usage message.
> I think that can fit in the commit msg with something like this:
> 
> daxctl: Remove unused options in create-device usage message

Sounds good to me.


> 
> I'm not familiar with this style of patch 2 being a reply to patch 1.
> Is there a reason this is not presented as a patchset?

Do you mean these 2 patches should post separately? or squash these
2 patches into a single one.




> 
> Thanks,
> Alison
> 
> 
>> ---
>> V2: make the usage match the manual because the usage is wrong.
>> ---
>>   daxctl/device.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/daxctl/device.c b/daxctl/device.c
>> index ffabd6cf5707..781dc4007f83 100644
>> --- a/daxctl/device.c
>> +++ b/daxctl/device.c
>> @@ -98,8 +98,6 @@ OPT_BOOLEAN('\0', "no-movable", &param.no_movable, \
>>   static const struct option create_options[] = {
>>   	BASE_OPTIONS(),
>>   	CREATE_OPTIONS(),
>> -	RECONFIG_OPTIONS(),
>> -	ZONE_OPTIONS(),
>>   	OPT_END(),
>>   };
>>   
>> -- 
>> 2.29.2
>>
>>
> 

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

[Verma, Vishal L]

On Fri, 2024-05-31 at 14:29 +0800, Li Zhijian wrote:
> Previously, the extra parameters will be ignored quietly, which is a bit
> weird and confusing.
> $ 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>

Reviewed-by: Vishal Verma <vishal.l.verma@intel.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;
>  	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;
>  	}

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

[Dave Jiang]

On 5/30/24 11:29 PM, Li Zhijian wrote:
> Previously, the extra parameters will be ignored quietly, which is a bit
> weird and confusing.
> $ 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>

Reviewed-by: Dave Jiang <dave.jiang@intel.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;
>  	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;
>  	}

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

[Alison Schofield]

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


> $ 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
> 
> 

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

[Zhijian Li (Fujitsu)]

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
>>
>>