| Message ID | 20240531062959.881772-1-lizhijian@fujitsu.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Ira Weiny |
| Headers | show |
| Series | [ndctl,v2,1/2] daxctl: Fix create-device parameters parsing | expand |
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; > }
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; > }
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 > >
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 >> >>
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; }
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(-)