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

[Alison Schofield <alison.schofield@intel.com>]

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