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...@samsung.com>
>> Signed-off-by: Li Zhijian <lizhij...@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
>>
>>
