daxctl: Change region input type from INTEGER to STRING.

daxctl: Change region input type from INTEGER to STRING.

[Li, Redhairer]

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

Re: daxctl: Change region input type from INTEGER to STRING.

[Dan Williams]

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", &param.region_id, "restrict to the given
> region"), \
> +OPT_STRING('r', "region", &param.region, "region-id", "filter by
> region"), \
>  OPT_BOOLEAN('u', "human", &param.human, "use human friendly number
> formats"), \
>  OPT_BOOLEAN('v', "verbose", &param.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(&region->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", &param.region_id, "filter by
> region"),
> + OPT_STRING('r', "region", &param.region, "region-id",
> + "filter by region"),
>   OPT_STRING('d', "dev", &param.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", &region_id) == 1
> + && daxctl_region_get_id(region) == region_id)
> + return region;

Let's just add a "sscanf(ident, "region%d", &region_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

RE: daxctl: Change region input type from INTEGER to STRING.

[Li, Redhairer]

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", &param.region_id, "restrict to the given 
> region"), \
> +OPT_STRING('r', "region", &param.region, "region-id", "filter by
> region"), \
>  OPT_BOOLEAN('u', "human", &param.human, "use human friendly number 
> formats"), \  OPT_BOOLEAN('v', "verbose", &param.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(&region->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", &param.region_id, "filter by region"),
> + OPT_STRING('r', "region", &param.region, "region-id", "filter by 
> + region"),
>   OPT_STRING('d', "dev", &param.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", &region_id) == 1 && 
> + daxctl_region_get_id(region) == region_id) return region;

Let's just add a "sscanf(ident, "region%d", &region_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

RE: daxctl: Change region input type from INTEGER to STRING.

[Li, Redhairer]

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", &param.region_id, "restrict to the given 
> region"), \
> +OPT_STRING('r', "region", &param.region, "region-id", "filter by
> region"), \
>  OPT_BOOLEAN('u', "human", &param.human, "use human friendly number 
> formats"), \  OPT_BOOLEAN('v', "verbose", &param.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(&region->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", &param.region_id, "filter by region"),
> + OPT_STRING('r', "region", &param.region, "region-id", "filter by 
> + region"),
>   OPT_STRING('d', "dev", &param.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", &region_id) == 1 &&
> + daxctl_region_get_id(region) == region_id) return region;

Let's just add a "sscanf(ident, "region%d", &region_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

Re: daxctl: Change region input type from INTEGER to STRING.

[Dan Williams]

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