* daxctl: Change region input type from INTEGER to STRING. @ 2019-11-21 9:18 Li, Redhairer 2019-11-26 19:21 ` Dan Williams 0 siblings, 1 reply; 5+ messages in thread From: Li, Redhairer @ 2019-11-21 9:18 UTC (permalink / raw) To: linux-nvdimm; +Cc: Williams, Dan J SHA-1: 66f34cdc26c58143fb8f11813dae98257b19ddc5 * daxctl: Change region input type from INTEGER to STRING. daxctl use STRING to be region input type. It makes daxctl can accept both <region-id> and region name as region parameter eg. daxctl list -r region5 daxctl list -r 5 Link: https://github.com/pmem/ndctl/issues/109 Signed-off-by: Redhairer Li <redhairer.li@intel.com> _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: daxctl: Change region input type from INTEGER to STRING. 2019-11-21 9:18 daxctl: Change region input type from INTEGER to STRING Li, Redhairer @ 2019-11-26 19:21 ` Dan Williams 2019-11-28 6:56 ` Li, Redhairer 0 siblings, 1 reply; 5+ messages in thread From: Dan Williams @ 2019-11-26 19:21 UTC (permalink / raw) To: Li, Redhairer; +Cc: linux-nvdimm Hi Redhairer, Thanks for submitting this first patch! Some feedback for submitting patches by email. The first is to make sure you are sending the patch in plain text, this one was in html format. The patch also needs to be included in the mail directly so the review can be done inline. This is what tools like "git send-email" or "stg mail" will do for you. Some more comments below where I pasted the patch manually: On Thu, Nov 21, 2019 at 1:18 AM Li, Redhairer <redhairer.li@intel.com> wrote: > > > > SHA-1: 66f34cdc26c58143fb8f11813dae98257b19ddc5 > > > > * daxctl: Change region input type from INTEGER to STRING. > > > > daxctl use STRING to be region input type. It makes daxctl can accept both <region-id> and region name as region parameter > > eg. > > daxctl list -r region5 > > daxctl list -r 5 > > > > Link: https://github.com/pmem/ndctl/issues/109 > > Signed-off-by: Redhairer Li <redhairer.li@intel.com> > > My mail reader has line wrapped this quotation below, "git send-email" and "stg mail" will help with that problem: From 66f34cdc26c58143fb8f11813dae98257b19ddc5 Mon Sep 17 00:00:00 > 2001 > From: redhairer <redhairer.li@intel.com> > Date: Thu, 21 Nov 2019 17:10:21 +0800 > Subject: [PATCH] daxctl: Change region input type from INTEGER to > STRING. > > daxctl use STRING to be region input type. It makes daxctl can accept > both <region-id> and region name as region parameter This line went past 80 columns. You can set your text editor to wrap at 80 lines. > eg. > daxctl list -r region5 > daxctl list -r 5 > > Link: https://github.com/pmem/ndctl/issues/109 > Signed-off-by: Redhairer Li <redhairer.li@intel.com> > --- > daxctl/device.c | 11 ++++------- > daxctl/lib/libdaxctl-private.h | 1 + > daxctl/lib/libdaxctl.c | 6 ++++++ > daxctl/lib/libdaxctl.sym | 1 + > daxctl/libdaxctl.h | 1 + > daxctl/list.c | 14 ++++++-------- > util/filter.c | 20 ++++++++++++++++++++ > util/filter.h | 2 ++ > util/sysfs.h | 6 ++++++ > 9 files changed, 47 insertions(+), 15 deletions(-) > > diff --git a/daxctl/device.c b/daxctl/device.c > index 72e506e..d9db2f9 100644 > --- a/daxctl/device.c > +++ b/daxctl/device.c > @@ -19,15 +19,13 @@ > static struct { > const char *dev; > const char *mode; > - int region_id; > + const char *region; > bool no_online; > bool no_movable; > bool force; > bool human; > bool verbose; > -} param = { > - .region_id = -1, > -}; > +} param; > > enum dev_mode { > DAXCTL_DEV_MODE_UNKNOWN, > @@ -51,7 +49,7 @@ enum device_action { > }; > > #define BASE_OPTIONS() \ > -OPT_INTEGER('r', "region", ¶m.region_id, "restrict to the given > region"), \ > +OPT_STRING('r', "region", ¶m.region, "region-id", "filter by > region"), \ > OPT_BOOLEAN('u', "human", ¶m.human, "use human friendly number > formats"), \ > OPT_BOOLEAN('v', "verbose", ¶m.verbose, "emit more debug > messages") > > @@ -484,8 +482,7 @@ static int do_xaction_device(const char *device, > enum device_action action, > *processed = 0; > > daxctl_region_foreach(ctx, region) { > - if (param.region_id >= 0 && param.region_id > - != daxctl_region_get_id(region)) > + if (!util_daxctl_region_filter(region, device)) > continue; > > daxctl_dev_foreach(region, dev) { > diff --git a/daxctl/lib/libdaxctl-private.h b/daxctl/lib/libdaxctl- > private.h > index 9f9c70d..169a8b8 100644 > --- a/daxctl/lib/libdaxctl-private.h > +++ b/daxctl/lib/libdaxctl-private.h > @@ -80,6 +80,7 @@ struct daxctl_region { > uuid_t uuid; > int refcount; > char *devname; > + char *regionname; So I don't think that "regionname" should be a property of 'struct daxctl_region' because there is no kernel device with that name for daxctl regions. I think this region name should only exist as a special case for "daxctl list". > size_t buf_len; > void *region_buf; > int devices_init; > diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c > index ee4a069..59697cd 100644 > --- a/daxctl/lib/libdaxctl.c > +++ b/daxctl/lib/libdaxctl.c > @@ -287,6 +287,7 @@ static struct daxctl_region *add_dax_region(void > *parent, int id, > region->refcount = 1; > list_head_init(®ion->devices); > region->devname = strdup(devpath_to_devname(base)); > + region->regionname = strdup(devpath_to_regionname(base)); > > sprintf(path, "%s/%s/size", base, attrs); > if (sysfs_read_attr(ctx, path, buf) == 0) > @@ -553,6 +554,11 @@ DAXCTL_EXPORT const char > *daxctl_region_get_devname(struct daxctl_region *region > return region->devname; > } > > +DAXCTL_EXPORT const char *daxctl_region_get_regionname(struct > daxctl_region *region) > +{ > + return region->regionname; > +} > + > DAXCTL_EXPORT const char *daxctl_region_get_path(struct > daxctl_region *region) > { API users should be aware that the region identifier is just a number, so I don't think we need these additions. > return region->region_path; > diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym > index 87d0236..13e5aec 100644 > --- a/daxctl/lib/libdaxctl.sym > +++ b/daxctl/lib/libdaxctl.sym > @@ -35,6 +35,7 @@ LIBDAXCTL_3 { > global: > daxctl_region_get_available_size; > daxctl_region_get_devname; > + daxctl_region_get_regionname; > daxctl_region_get_dev_seed; > } LIBDAXCTL_2; > > diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h > index 6c545e1..1592c2f 100644 > --- a/daxctl/libdaxctl.h > +++ b/daxctl/libdaxctl.h > @@ -54,6 +54,7 @@ unsigned long long > daxctl_region_get_available_size( > unsigned long long daxctl_region_get_size(struct daxctl_region > *region); > unsigned long daxctl_region_get_align(struct daxctl_region *region); > const char *daxctl_region_get_devname(struct daxctl_region *region); > +const char *daxctl_region_get_regionname(struct daxctl_region > *region); > const char *daxctl_region_get_path(struct daxctl_region *region); > > struct daxctl_dev *daxctl_region_get_dev_seed(struct daxctl_region > *region); > diff --git a/daxctl/list.c b/daxctl/list.c > index e56300d..6c6251b 100644 > --- a/daxctl/list.c > +++ b/daxctl/list.c > @@ -44,10 +44,8 @@ static unsigned long listopts_to_flags(void) > > static struct { > const char *dev; > - int region_id; > -} param = { > - .region_id = -1, > -}; > + const char *region; > +} param; > > static int did_fail; > > @@ -66,7 +64,8 @@ static int num_list_flags(void) > int cmd_list(int argc, const char **argv, struct daxctl_ctx *ctx) > { > const struct option options[] = { > - OPT_INTEGER('r', "region", ¶m.region_id, "filter by > region"), > + OPT_STRING('r', "region", ¶m.region, "region-id", > + "filter by region"), > OPT_STRING('d', "dev", ¶m.dev, "dev-id", > "filter by dax device instance name"), > OPT_BOOLEAN('D', "devices", &list.devs, "include dax > device info"), > @@ -94,7 +93,7 @@ int cmd_list(int argc, const char **argv, struct > daxctl_ctx *ctx) > usage_with_options(u, options); > > if (num_list_flags() == 0) { > - list.regions = param.region_id >= 0; > + list.regions = !!param.region; > list.devs = !!param.dev; > } > > @@ -106,8 +105,7 @@ int cmd_list(int argc, const char **argv, struct > daxctl_ctx *ctx) > daxctl_region_foreach(ctx, region) { > struct json_object *jregion = NULL; > > - if (param.region_id >= 0 && param.region_id > - != daxctl_region_get_id(region)) > + if (!util_daxctl_region_filter(region, param.region)) > continue; > > if (list.regions) { > diff --git a/util/filter.c b/util/filter.c > index 1734bce..da647a8 100644 > --- a/util/filter.c > +++ b/util/filter.c > @@ -335,6 +335,26 @@ struct daxctl_dev *util_daxctl_dev_filter(struct > daxctl_dev *dev, > return NULL; > } > > +struct daxctl_region *util_daxctl_region_filter(struct daxctl_region > *region, > + const char *ident) > +{ > + int region_id; > + const char *region_name; > + > + if (!ident || strcmp(ident, "all") == 0) > + return region; > + > + if (sscanf(ident, "%d", ®ion_id) == 1 > + && daxctl_region_get_id(region) == region_id) > + return region; Let's just add a "sscanf(ident, "region%d", ®ion_id)" in the case the above ident does not match. > + > + region_name = daxctl_region_get_regionname(region); > + if (strcmp(region_name, ident)==0) > + return region; ...with the above new sscanf then daxctl_region_get_regionname() is not needed. > + > + return NULL; > +} > + > static enum ndctl_namespace_mode mode_to_type(const char *mode) > { > if (!mode) > diff --git a/util/filter.h b/util/filter.h > index c2cdddf..0c12b94 100644 > --- a/util/filter.h > +++ b/util/filter.h > @@ -37,6 +37,8 @@ struct ndctl_region > *util_region_filter_by_namespace(struct ndctl_region *region > const char *ident); > struct daxctl_dev *util_daxctl_dev_filter(struct daxctl_dev *dev, > const char *ident); > +struct daxctl_region *util_daxctl_region_filter(struct daxctl_region > *region, > + const char *ident); > > struct json_object; > > diff --git a/util/sysfs.h b/util/sysfs.h > index fb169c6..84c0965 100644 > --- a/util/sysfs.h > +++ b/util/sysfs.h > @@ -37,4 +37,10 @@ static inline const char *devpath_to_devname(const > char *devpath) > { > return strrchr(devpath, '/') + 1; > } > + > +static inline const char *devpath_to_regionname(const char *devpath) > +{ > + char* tmp_devpath = strdup(devpath); > + return strtok(strstr(tmp_devpath, "region"), "/"); Another reason to not create "daxctl_region_get_regionname" is that not all daxctl devices will have "region" in the devpath. _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: daxctl: Change region input type from INTEGER to STRING. 2019-11-26 19:21 ` Dan Williams @ 2019-11-28 6:56 ` Li, Redhairer 2019-12-03 2:38 ` Li, Redhairer 0 siblings, 1 reply; 5+ messages in thread From: Li, Redhairer @ 2019-11-28 6:56 UTC (permalink / raw) To: Williams, Dan J; +Cc: linux-nvdimm Hi Dan, I have modified it by your feedback. SHA-1: 00abd19ad64a4ac2f352cbeb1d2fabcb3ffa10ea * * daxctl: Change region input type from INTEGER to STRING. daxctl use STRING to be region input type. It makes daxctl can accept both <region-id> and region name as region parameter eg. daxctl list -r region5 daxctl list -r 5 Link: https://github.com/pmem/ndctl/issues/109 Signed-off-by: Redhairer Li <redhairer.li@intel.com> -----Original Message----- From: Dan Williams <dan.j.williams@intel.com> Sent: Wednesday, November 27, 2019 3:21 AM To: Li, Redhairer <redhairer.li@intel.com> Cc: linux-nvdimm@lists.01.org Subject: Re: daxctl: Change region input type from INTEGER to STRING. Hi Redhairer, Thanks for submitting this first patch! Some feedback for submitting patches by email. The first is to make sure you are sending the patch in plain text, this one was in html format. The patch also needs to be included in the mail directly so the review can be done inline. This is what tools like "git send-email" or "stg mail" will do for you. Some more comments below where I pasted the patch manually: On Thu, Nov 21, 2019 at 1:18 AM Li, Redhairer <redhairer.li@intel.com> wrote: > > > > SHA-1: 66f34cdc26c58143fb8f11813dae98257b19ddc5 > > > > * daxctl: Change region input type from INTEGER to STRING. > > > > daxctl use STRING to be region input type. It makes daxctl can accept > both <region-id> and region name as region parameter > > eg. > > daxctl list -r region5 > > daxctl list -r 5 > > > > Link: https://github.com/pmem/ndctl/issues/109 > > Signed-off-by: Redhairer Li <redhairer.li@intel.com> > > My mail reader has line wrapped this quotation below, "git send-email" and "stg mail" will help with that problem: From 66f34cdc26c58143fb8f11813dae98257b19ddc5 Mon Sep 17 00:00:00 > 2001 > From: redhairer <redhairer.li@intel.com> > Date: Thu, 21 Nov 2019 17:10:21 +0800 > Subject: [PATCH] daxctl: Change region input type from INTEGER to > STRING. > > daxctl use STRING to be region input type. It makes daxctl can accept > both <region-id> and region name as region parameter This line went past 80 columns. You can set your text editor to wrap at 80 lines. > eg. > daxctl list -r region5 > daxctl list -r 5 > > Link: https://github.com/pmem/ndctl/issues/109 > Signed-off-by: Redhairer Li <redhairer.li@intel.com> > --- > daxctl/device.c | 11 ++++------- > daxctl/lib/libdaxctl-private.h | 1 + > daxctl/lib/libdaxctl.c | 6 ++++++ > daxctl/lib/libdaxctl.sym | 1 + > daxctl/libdaxctl.h | 1 + > daxctl/list.c | 14 ++++++-------- > util/filter.c | 20 ++++++++++++++++++++ > util/filter.h | 2 ++ > util/sysfs.h | 6 ++++++ > 9 files changed, 47 insertions(+), 15 deletions(-) > > diff --git a/daxctl/device.c b/daxctl/device.c index 72e506e..d9db2f9 > 100644 > --- a/daxctl/device.c > +++ b/daxctl/device.c > @@ -19,15 +19,13 @@ > static struct { > const char *dev; > const char *mode; > - int region_id; > + const char *region; > bool no_online; > bool no_movable; > bool force; > bool human; > bool verbose; > -} param = { > - .region_id = -1, > -}; > +} param; > > enum dev_mode { > DAXCTL_DEV_MODE_UNKNOWN, > @@ -51,7 +49,7 @@ enum device_action { }; > > #define BASE_OPTIONS() \ > -OPT_INTEGER('r', "region", ¶m.region_id, "restrict to the given > region"), \ > +OPT_STRING('r', "region", ¶m.region, "region-id", "filter by > region"), \ > OPT_BOOLEAN('u', "human", ¶m.human, "use human friendly number > formats"), \ OPT_BOOLEAN('v', "verbose", ¶m.verbose, "emit more > debug > messages") > > @@ -484,8 +482,7 @@ static int do_xaction_device(const char *device, > enum device_action action, > *processed = 0; > > daxctl_region_foreach(ctx, region) { > - if (param.region_id >= 0 && param.region_id > - != daxctl_region_get_id(region)) > + if (!util_daxctl_region_filter(region, device)) > continue; > > daxctl_dev_foreach(region, dev) { > diff --git a/daxctl/lib/libdaxctl-private.h b/daxctl/lib/libdaxctl- > private.h index 9f9c70d..169a8b8 100644 > --- a/daxctl/lib/libdaxctl-private.h > +++ b/daxctl/lib/libdaxctl-private.h > @@ -80,6 +80,7 @@ struct daxctl_region { > uuid_t uuid; > int refcount; > char *devname; > + char *regionname; So I don't think that "regionname" should be a property of 'struct daxctl_region' because there is no kernel device with that name for daxctl regions. I think this region name should only exist as a special case for "daxctl list". > size_t buf_len; > void *region_buf; > int devices_init; > diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index > ee4a069..59697cd 100644 > --- a/daxctl/lib/libdaxctl.c > +++ b/daxctl/lib/libdaxctl.c > @@ -287,6 +287,7 @@ static struct daxctl_region *add_dax_region(void > *parent, int id, > region->refcount = 1; > list_head_init(®ion->devices); > region->devname = strdup(devpath_to_devname(base)); > + region->regionname = strdup(devpath_to_regionname(base)); > > sprintf(path, "%s/%s/size", base, attrs); > if (sysfs_read_attr(ctx, path, buf) == 0) @@ -553,6 +554,11 @@ > DAXCTL_EXPORT const char *daxctl_region_get_devname(struct > daxctl_region *region > return region->devname; > } > > +DAXCTL_EXPORT const char *daxctl_region_get_regionname(struct > daxctl_region *region) > +{ > + return region->regionname; > +} > + > DAXCTL_EXPORT const char *daxctl_region_get_path(struct daxctl_region > *region) { API users should be aware that the region identifier is just a number, so I don't think we need these additions. > return region->region_path; > diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym index > 87d0236..13e5aec 100644 > --- a/daxctl/lib/libdaxctl.sym > +++ b/daxctl/lib/libdaxctl.sym > @@ -35,6 +35,7 @@ LIBDAXCTL_3 { > global: > daxctl_region_get_available_size; > daxctl_region_get_devname; > + daxctl_region_get_regionname; > daxctl_region_get_dev_seed; > } LIBDAXCTL_2; > > diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h index > 6c545e1..1592c2f 100644 > --- a/daxctl/libdaxctl.h > +++ b/daxctl/libdaxctl.h > @@ -54,6 +54,7 @@ unsigned long long > daxctl_region_get_available_size( > unsigned long long daxctl_region_get_size(struct daxctl_region > *region); unsigned long daxctl_region_get_align(struct daxctl_region > *region); const char *daxctl_region_get_devname(struct daxctl_region > *region); > +const char *daxctl_region_get_regionname(struct daxctl_region > *region); > const char *daxctl_region_get_path(struct daxctl_region *region); > > struct daxctl_dev *daxctl_region_get_dev_seed(struct daxctl_region > *region); diff --git a/daxctl/list.c b/daxctl/list.c index > e56300d..6c6251b 100644 > --- a/daxctl/list.c > +++ b/daxctl/list.c > @@ -44,10 +44,8 @@ static unsigned long listopts_to_flags(void) > > static struct { > const char *dev; > - int region_id; > -} param = { > - .region_id = -1, > -}; > + const char *region; > +} param; > > static int did_fail; > > @@ -66,7 +64,8 @@ static int num_list_flags(void) int cmd_list(int > argc, const char **argv, struct daxctl_ctx *ctx) { > const struct option options[] = { > - OPT_INTEGER('r', "region", ¶m.region_id, "filter by region"), > + OPT_STRING('r', "region", ¶m.region, "region-id", "filter by > + region"), > OPT_STRING('d', "dev", ¶m.dev, "dev-id", > "filter by dax device instance name"), > OPT_BOOLEAN('D', "devices", &list.devs, "include dax device info"), > @@ -94,7 +93,7 @@ int cmd_list(int argc, const char **argv, struct > daxctl_ctx *ctx) > usage_with_options(u, options); > > if (num_list_flags() == 0) { > - list.regions = param.region_id >= 0; > + list.regions = !!param.region; > list.devs = !!param.dev; > } > > @@ -106,8 +105,7 @@ int cmd_list(int argc, const char **argv, struct > daxctl_ctx *ctx) > daxctl_region_foreach(ctx, region) { > struct json_object *jregion = NULL; > > - if (param.region_id >= 0 && param.region_id > - != daxctl_region_get_id(region)) > + if (!util_daxctl_region_filter(region, param.region)) > continue; > > if (list.regions) { > diff --git a/util/filter.c b/util/filter.c index 1734bce..da647a8 > 100644 > --- a/util/filter.c > +++ b/util/filter.c > @@ -335,6 +335,26 @@ struct daxctl_dev *util_daxctl_dev_filter(struct > daxctl_dev *dev, > return NULL; > } > > +struct daxctl_region *util_daxctl_region_filter(struct daxctl_region > *region, > + const char *ident) > +{ > + int region_id; > + const char *region_name; > + > + if (!ident || strcmp(ident, "all") == 0) return region; > + > + if (sscanf(ident, "%d", ®ion_id) == 1 && > + daxctl_region_get_id(region) == region_id) return region; Let's just add a "sscanf(ident, "region%d", ®ion_id)" in the case the above ident does not match. > + > + region_name = daxctl_region_get_regionname(region); > + if (strcmp(region_name, ident)==0) > + return region; ...with the above new sscanf then daxctl_region_get_regionname() is not needed. > + > + return NULL; > +} > + > static enum ndctl_namespace_mode mode_to_type(const char *mode) { > if (!mode) > diff --git a/util/filter.h b/util/filter.h index c2cdddf..0c12b94 > 100644 > --- a/util/filter.h > +++ b/util/filter.h > @@ -37,6 +37,8 @@ struct ndctl_region > *util_region_filter_by_namespace(struct ndctl_region *region > const char *ident); > struct daxctl_dev *util_daxctl_dev_filter(struct daxctl_dev *dev, > const char *ident); > +struct daxctl_region *util_daxctl_region_filter(struct daxctl_region > *region, > + const char *ident); > > struct json_object; > > diff --git a/util/sysfs.h b/util/sysfs.h index fb169c6..84c0965 100644 > --- a/util/sysfs.h > +++ b/util/sysfs.h > @@ -37,4 +37,10 @@ static inline const char *devpath_to_devname(const > char *devpath) { > return strrchr(devpath, '/') + 1; > } > + > +static inline const char *devpath_to_regionname(const char *devpath) > +{ > + char* tmp_devpath = strdup(devpath); return > +strtok(strstr(tmp_devpath, "region"), "/"); Another reason to not create "daxctl_region_get_regionname" is that not all daxctl devices will have "region" in the devpath. _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: daxctl: Change region input type from INTEGER to STRING. 2019-11-28 6:56 ` Li, Redhairer @ 2019-12-03 2:38 ` Li, Redhairer 2019-12-04 1:50 ` Dan Williams 0 siblings, 1 reply; 5+ messages in thread From: Li, Redhairer @ 2019-12-03 2:38 UTC (permalink / raw) To: Williams, Dan J; +Cc: 'linux-nvdimm@lists.01.org' I have squashed my previous two change. Please refer it. -----Original Message----- From: Li, Redhairer Sent: Thursday, November 28, 2019 2:56 PM To: Dan Williams <dan.j.williams@intel.com> Cc: linux-nvdimm@lists.01.org Subject: RE: daxctl: Change region input type from INTEGER to STRING. Hi Dan, I have modified it by your feedback. SHA-1: 00abd19ad64a4ac2f352cbeb1d2fabcb3ffa10ea * * daxctl: Change region input type from INTEGER to STRING. daxctl use STRING to be region input type. It makes daxctl can accept both <region-id> and region name as region parameter eg. daxctl list -r region5 daxctl list -r 5 Link: https://github.com/pmem/ndctl/issues/109 Signed-off-by: Redhairer Li <redhairer.li@intel.com> -----Original Message----- From: Dan Williams <dan.j.williams@intel.com> Sent: Wednesday, November 27, 2019 3:21 AM To: Li, Redhairer <redhairer.li@intel.com> Cc: linux-nvdimm@lists.01.org Subject: Re: daxctl: Change region input type from INTEGER to STRING. Hi Redhairer, Thanks for submitting this first patch! Some feedback for submitting patches by email. The first is to make sure you are sending the patch in plain text, this one was in html format. The patch also needs to be included in the mail directly so the review can be done inline. This is what tools like "git send-email" or "stg mail" will do for you. Some more comments below where I pasted the patch manually: On Thu, Nov 21, 2019 at 1:18 AM Li, Redhairer <redhairer.li@intel.com> wrote: > > > > SHA-1: 66f34cdc26c58143fb8f11813dae98257b19ddc5 > > > > * daxctl: Change region input type from INTEGER to STRING. > > > > daxctl use STRING to be region input type. It makes daxctl can accept > both <region-id> and region name as region parameter > > eg. > > daxctl list -r region5 > > daxctl list -r 5 > > > > Link: https://github.com/pmem/ndctl/issues/109 > > Signed-off-by: Redhairer Li <redhairer.li@intel.com> > > My mail reader has line wrapped this quotation below, "git send-email" and "stg mail" will help with that problem: From 66f34cdc26c58143fb8f11813dae98257b19ddc5 Mon Sep 17 00:00:00 > 2001 > From: redhairer <redhairer.li@intel.com> > Date: Thu, 21 Nov 2019 17:10:21 +0800 > Subject: [PATCH] daxctl: Change region input type from INTEGER to > STRING. > > daxctl use STRING to be region input type. It makes daxctl can accept > both <region-id> and region name as region parameter This line went past 80 columns. You can set your text editor to wrap at 80 lines. > eg. > daxctl list -r region5 > daxctl list -r 5 > > Link: https://github.com/pmem/ndctl/issues/109 > Signed-off-by: Redhairer Li <redhairer.li@intel.com> > --- > daxctl/device.c | 11 ++++------- > daxctl/lib/libdaxctl-private.h | 1 + > daxctl/lib/libdaxctl.c | 6 ++++++ > daxctl/lib/libdaxctl.sym | 1 + > daxctl/libdaxctl.h | 1 + > daxctl/list.c | 14 ++++++-------- > util/filter.c | 20 ++++++++++++++++++++ > util/filter.h | 2 ++ > util/sysfs.h | 6 ++++++ > 9 files changed, 47 insertions(+), 15 deletions(-) > > diff --git a/daxctl/device.c b/daxctl/device.c index 72e506e..d9db2f9 > 100644 > --- a/daxctl/device.c > +++ b/daxctl/device.c > @@ -19,15 +19,13 @@ > static struct { > const char *dev; > const char *mode; > - int region_id; > + const char *region; > bool no_online; > bool no_movable; > bool force; > bool human; > bool verbose; > -} param = { > - .region_id = -1, > -}; > +} param; > > enum dev_mode { > DAXCTL_DEV_MODE_UNKNOWN, > @@ -51,7 +49,7 @@ enum device_action { }; > > #define BASE_OPTIONS() \ > -OPT_INTEGER('r', "region", ¶m.region_id, "restrict to the given > region"), \ > +OPT_STRING('r', "region", ¶m.region, "region-id", "filter by > region"), \ > OPT_BOOLEAN('u', "human", ¶m.human, "use human friendly number > formats"), \ OPT_BOOLEAN('v', "verbose", ¶m.verbose, "emit more > debug > messages") > > @@ -484,8 +482,7 @@ static int do_xaction_device(const char *device, > enum device_action action, > *processed = 0; > > daxctl_region_foreach(ctx, region) { > - if (param.region_id >= 0 && param.region_id > - != daxctl_region_get_id(region)) > + if (!util_daxctl_region_filter(region, device)) > continue; > > daxctl_dev_foreach(region, dev) { > diff --git a/daxctl/lib/libdaxctl-private.h b/daxctl/lib/libdaxctl- > private.h index 9f9c70d..169a8b8 100644 > --- a/daxctl/lib/libdaxctl-private.h > +++ b/daxctl/lib/libdaxctl-private.h > @@ -80,6 +80,7 @@ struct daxctl_region { > uuid_t uuid; > int refcount; > char *devname; > + char *regionname; So I don't think that "regionname" should be a property of 'struct daxctl_region' because there is no kernel device with that name for daxctl regions. I think this region name should only exist as a special case for "daxctl list". > size_t buf_len; > void *region_buf; > int devices_init; > diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index > ee4a069..59697cd 100644 > --- a/daxctl/lib/libdaxctl.c > +++ b/daxctl/lib/libdaxctl.c > @@ -287,6 +287,7 @@ static struct daxctl_region *add_dax_region(void > *parent, int id, > region->refcount = 1; > list_head_init(®ion->devices); > region->devname = strdup(devpath_to_devname(base)); > + region->regionname = strdup(devpath_to_regionname(base)); > > sprintf(path, "%s/%s/size", base, attrs); > if (sysfs_read_attr(ctx, path, buf) == 0) @@ -553,6 +554,11 @@ > DAXCTL_EXPORT const char *daxctl_region_get_devname(struct > daxctl_region *region > return region->devname; > } > > +DAXCTL_EXPORT const char *daxctl_region_get_regionname(struct > daxctl_region *region) > +{ > + return region->regionname; > +} > + > DAXCTL_EXPORT const char *daxctl_region_get_path(struct daxctl_region > *region) { API users should be aware that the region identifier is just a number, so I don't think we need these additions. > return region->region_path; > diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym index > 87d0236..13e5aec 100644 > --- a/daxctl/lib/libdaxctl.sym > +++ b/daxctl/lib/libdaxctl.sym > @@ -35,6 +35,7 @@ LIBDAXCTL_3 { > global: > daxctl_region_get_available_size; > daxctl_region_get_devname; > + daxctl_region_get_regionname; > daxctl_region_get_dev_seed; > } LIBDAXCTL_2; > > diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h index > 6c545e1..1592c2f 100644 > --- a/daxctl/libdaxctl.h > +++ b/daxctl/libdaxctl.h > @@ -54,6 +54,7 @@ unsigned long long > daxctl_region_get_available_size( > unsigned long long daxctl_region_get_size(struct daxctl_region > *region); unsigned long daxctl_region_get_align(struct daxctl_region > *region); const char *daxctl_region_get_devname(struct daxctl_region > *region); > +const char *daxctl_region_get_regionname(struct daxctl_region > *region); > const char *daxctl_region_get_path(struct daxctl_region *region); > > struct daxctl_dev *daxctl_region_get_dev_seed(struct daxctl_region > *region); diff --git a/daxctl/list.c b/daxctl/list.c index > e56300d..6c6251b 100644 > --- a/daxctl/list.c > +++ b/daxctl/list.c > @@ -44,10 +44,8 @@ static unsigned long listopts_to_flags(void) > > static struct { > const char *dev; > - int region_id; > -} param = { > - .region_id = -1, > -}; > + const char *region; > +} param; > > static int did_fail; > > @@ -66,7 +64,8 @@ static int num_list_flags(void) int cmd_list(int > argc, const char **argv, struct daxctl_ctx *ctx) { > const struct option options[] = { > - OPT_INTEGER('r', "region", ¶m.region_id, "filter by region"), > + OPT_STRING('r', "region", ¶m.region, "region-id", "filter by > + region"), > OPT_STRING('d', "dev", ¶m.dev, "dev-id", > "filter by dax device instance name"), > OPT_BOOLEAN('D', "devices", &list.devs, "include dax device info"), > @@ -94,7 +93,7 @@ int cmd_list(int argc, const char **argv, struct > daxctl_ctx *ctx) > usage_with_options(u, options); > > if (num_list_flags() == 0) { > - list.regions = param.region_id >= 0; > + list.regions = !!param.region; > list.devs = !!param.dev; > } > > @@ -106,8 +105,7 @@ int cmd_list(int argc, const char **argv, struct > daxctl_ctx *ctx) > daxctl_region_foreach(ctx, region) { > struct json_object *jregion = NULL; > > - if (param.region_id >= 0 && param.region_id > - != daxctl_region_get_id(region)) > + if (!util_daxctl_region_filter(region, param.region)) > continue; > > if (list.regions) { > diff --git a/util/filter.c b/util/filter.c index 1734bce..da647a8 > 100644 > --- a/util/filter.c > +++ b/util/filter.c > @@ -335,6 +335,26 @@ struct daxctl_dev *util_daxctl_dev_filter(struct > daxctl_dev *dev, > return NULL; > } > > +struct daxctl_region *util_daxctl_region_filter(struct daxctl_region > *region, > + const char *ident) > +{ > + int region_id; > + const char *region_name; > + > + if (!ident || strcmp(ident, "all") == 0) return region; > + > + if (sscanf(ident, "%d", ®ion_id) == 1 && > + daxctl_region_get_id(region) == region_id) return region; Let's just add a "sscanf(ident, "region%d", ®ion_id)" in the case the above ident does not match. > + > + region_name = daxctl_region_get_regionname(region); > + if (strcmp(region_name, ident)==0) > + return region; ...with the above new sscanf then daxctl_region_get_regionname() is not needed. > + > + return NULL; > +} > + > static enum ndctl_namespace_mode mode_to_type(const char *mode) { > if (!mode) > diff --git a/util/filter.h b/util/filter.h index c2cdddf..0c12b94 > 100644 > --- a/util/filter.h > +++ b/util/filter.h > @@ -37,6 +37,8 @@ struct ndctl_region > *util_region_filter_by_namespace(struct ndctl_region *region > const char *ident); > struct daxctl_dev *util_daxctl_dev_filter(struct daxctl_dev *dev, > const char *ident); > +struct daxctl_region *util_daxctl_region_filter(struct daxctl_region > *region, > + const char *ident); > > struct json_object; > > diff --git a/util/sysfs.h b/util/sysfs.h index fb169c6..84c0965 100644 > --- a/util/sysfs.h > +++ b/util/sysfs.h > @@ -37,4 +37,10 @@ static inline const char *devpath_to_devname(const > char *devpath) { > return strrchr(devpath, '/') + 1; > } > + > +static inline const char *devpath_to_regionname(const char *devpath) > +{ > + char* tmp_devpath = strdup(devpath); return > +strtok(strstr(tmp_devpath, "region"), "/"); Another reason to not create "daxctl_region_get_regionname" is that not all daxctl devices will have "region" in the devpath. _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: daxctl: Change region input type from INTEGER to STRING. 2019-12-03 2:38 ` Li, Redhairer @ 2019-12-04 1:50 ` Dan Williams 0 siblings, 0 replies; 5+ messages in thread From: Dan Williams @ 2019-12-04 1:50 UTC (permalink / raw) To: Li, Redhairer; +Cc: linux-nvdimm On Mon, Dec 2, 2019 at 6:43 PM Li, Redhairer <redhairer.li@intel.com> wrote: > > I have squashed my previous two change. > Please refer it. The logic looks good, and it's passing my manual tests. However the unit tests are currently failing, so I got sidetracked trying to look into that. Not your issue, but it prevented me from responding right away. As for the final version of the patch: The changelog grammar needs a small fixup. How about: " Allow daxctl to accept both <region-id>, and region name as region parameter. For example: daxctl list -r region5 daxctl list -r 5 " There were some whitespace errors in util_daxctl_region_filter() (indentation for a continued if statement and missing space around "=="). You can use vim's default C identing. It also needs to be resent as a plain text patch, not an attachment, otherwise patchwork won't pick it up [1]. For this first submission you can send it again as an attachment and I'll resend to the list. [1]: https://patchwork.kernel.org/project/linux-nvdimm/list/ _______________________________________________ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-leave@lists.01.org ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-04 1:50 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-21 9:18 daxctl: Change region input type from INTEGER to STRING Li, Redhairer 2019-11-26 19:21 ` Dan Williams 2019-11-28 6:56 ` Li, Redhairer 2019-12-03 2:38 ` Li, Redhairer 2019-12-04 1:50 ` Dan Williams
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).