diff mbox

[ndctl,v2] ndctl, list: display the 'map' location in listings

Message ID 20180529231503.31710-1-vishal.l.verma@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Verma, Vishal L May 29, 2018, 11:15 p.m. UTC
For 'fsdax' and 'devdax' namespaces, a 'map' location may be specified
for page structures storage. This can be 'mem', for system RAM, or 'dev'
for using pmem as the backing storage. Once set, there was no way of
telling using ndctl, which of the two locations a namespace was
configured for. Add this in util_namespace_to_json so that all
namespace listings contain the map location.

Reported-by: "Yigal Korman" <yigal.korman@netapp.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 util/json.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

v2: Also account for memmap=ss!nn or legacy-e820 namespaces. (Dan)

Comments

Dan Williams June 2, 2018, 12:01 a.m. UTC | #1
On Tue, May 29, 2018 at 4:15 PM, Vishal Verma <vishal.l.verma@intel.com> wrote:
> For 'fsdax' and 'devdax' namespaces, a 'map' location may be specified
> for page structures storage. This can be 'mem', for system RAM, or 'dev'
> for using pmem as the backing storage. Once set, there was no way of
> telling using ndctl, which of the two locations a namespace was
> configured for. Add this in util_namespace_to_json so that all
> namespace listings contain the map location.
>
> Reported-by: "Yigal Korman" <yigal.korman@netapp.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  util/json.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
>
> v2: Also account for memmap=ss!nn or legacy-e820 namespaces. (Dan)
>
> diff --git a/util/json.c b/util/json.c
> index c606e1c..b020300 100644
> --- a/util/json.c
> +++ b/util/json.c
> @@ -667,11 +667,17 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
>  {
>         struct json_object *jndns = json_object_new_object();
>         struct json_object *jobj, *jbbs = NULL;
> +       const char *locations[] = {
> +               [NDCTL_PFN_LOC_NONE] = "none",
> +               [NDCTL_PFN_LOC_RAM] = "mem",
> +               [NDCTL_PFN_LOC_PMEM] = "dev",
> +       };
>         unsigned long long size = ULLONG_MAX;
>         unsigned int sector_size = UINT_MAX;
>         enum ndctl_namespace_mode mode;
>         const char *bdev = NULL, *name;
>         unsigned int bb_count = 0;
> +       enum ndctl_pfn_loc loc;

we could initialize loc to NDCTL_PFN_LOC_NONE here.

>         struct ndctl_btt *btt;
>         struct ndctl_pfn *pfn;
>         struct ndctl_dax *dax;
> @@ -693,33 +699,49 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
>         mode = ndctl_namespace_get_mode(ndns);
>         switch (mode) {
>         case NDCTL_NS_MODE_MEMORY:
> -               if (pfn) /* dynamic memory mode */
> +               jobj = json_object_new_string("fsdax");
> +               if (jobj)
> +                       json_object_object_add(jndns, "mode", jobj);
> +               loc = ndctl_pfn_get_location(pfn);

NULL ptr de-reference if pfn is NULL

> +               if (pfn) { /* dynamic memory mode */


>                         size = ndctl_pfn_get_size(pfn);
> -               else /* native/static memory mode */
> +                       jobj = json_object_new_string(locations[loc]);
> +               } else { /* native/static memory mode */
>                         size = ndctl_namespace_get_size(ndns);
> -               jobj = json_object_new_string("fsdax");
> +                       jobj = json_object_new_string("mem");

We could just set NDCTL_PFN_LOC_RAM here.

> +               }
> +               if (jobj)
> +                       json_object_object_add(jndns, "map", jobj);
>                 break;
>         case NDCTL_NS_MODE_DAX:
>                 if (!dax)
>                         goto err;
>                 size = ndctl_dax_get_size(dax);
>                 jobj = json_object_new_string("devdax");
> +               if (jobj)
> +                       json_object_object_add(jndns, "mode", jobj);
> +               loc = ndctl_dax_get_location(dax);
> +               jobj = json_object_new_string(locations[loc]);
> +               if (jobj)
> +                       json_object_object_add(jndns, "map", jobj);
>                 break;
>         case NDCTL_NS_MODE_SAFE:
>                 if (!btt)
>                         goto err;
>                 jobj = json_object_new_string("sector");
> +               if (jobj)
> +                       json_object_object_add(jndns, "mode", jobj);
>                 size = ndctl_btt_get_size(btt);
>                 break;
>         case NDCTL_NS_MODE_RAW:
>                 size = ndctl_namespace_get_size(ndns);
>                 jobj = json_object_new_string("raw");
> +               if (jobj)
> +                       json_object_object_add(jndns, "mode", jobj);
>                 break;
>         default:
>                 jobj = NULL;
>         }
> -       if (jobj)
> -               json_object_object_add(jndns, "mode", jobj);

Why is mode getting moved into each case?

I'd put the

    jobj = json_object_new_string(locations[loc]);
    if (jobj)
        json_object_object_add(jndns, "map", jobj);

...at the end here and just gate it on mode != NDCTL_NS_MODE_SAFE &&
mode != NDCTL_NS_MODE_RAW
Verma, Vishal L June 4, 2018, 11:21 p.m. UTC | #2
On Fri, 2018-06-01 at 17:01 -0700, Dan Williams wrote:
> On Tue, May 29, 2018 at 4:15 PM, Vishal Verma <vishal.l.verma@intel.com>
> wrote:
> > For 'fsdax' and 'devdax' namespaces, a 'map' location may be specified
> > for page structures storage. This can be 'mem', for system RAM, or
> > 'dev'
> > for using pmem as the backing storage. Once set, there was no way of
> > telling using ndctl, which of the two locations a namespace was
> > configured for. Add this in util_namespace_to_json so that all
> > namespace listings contain the map location.
> > 
> > Reported-by: "Yigal Korman" <yigal.korman@netapp.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  util/json.c | 32 +++++++++++++++++++++++++++-----
> >  1 file changed, 27 insertions(+), 5 deletions(-)
> > 
> > v2: Also account for memmap=ss!nn or legacy-e820 namespaces. (Dan)
> > 
> > diff --git a/util/json.c b/util/json.c
> > index c606e1c..b020300 100644
> > --- a/util/json.c
> > +++ b/util/json.c
> > @@ -667,11 +667,17 @@ struct json_object *util_namespace_to_json(struct
> > ndctl_namespace *ndns,
> >  {
> >         struct json_object *jndns = json_object_new_object();
> >         struct json_object *jobj, *jbbs = NULL;
> > +       const char *locations[] = {
> > +               [NDCTL_PFN_LOC_NONE] = "none",
> > +               [NDCTL_PFN_LOC_RAM] = "mem",
> > +               [NDCTL_PFN_LOC_PMEM] = "dev",
> > +       };
> >         unsigned long long size = ULLONG_MAX;
> >         unsigned int sector_size = UINT_MAX;
> >         enum ndctl_namespace_mode mode;
> >         const char *bdev = NULL, *name;
> >         unsigned int bb_count = 0;
> > +       enum ndctl_pfn_loc loc;
> 
> we could initialize loc to NDCTL_PFN_LOC_NONE here.
> 
> >         struct ndctl_btt *btt;
> >         struct ndctl_pfn *pfn;
> >         struct ndctl_dax *dax;
> > @@ -693,33 +699,49 @@ struct json_object *util_namespace_to_json(struct
> > ndctl_namespace *ndns,
> >         mode = ndctl_namespace_get_mode(ndns);
> >         switch (mode) {
> >         case NDCTL_NS_MODE_MEMORY:
> > -               if (pfn) /* dynamic memory mode */
> > +               jobj = json_object_new_string("fsdax");
> > +               if (jobj)
> > +                       json_object_object_add(jndns, "mode", jobj);
> > +               loc = ndctl_pfn_get_location(pfn);
> 
> NULL ptr de-reference if pfn is NULL
> 
> > +               if (pfn) { /* dynamic memory mode */
> 
> 
> >                         size = ndctl_pfn_get_size(pfn);
> > -               else /* native/static memory mode */
> > +                       jobj = json_object_new_string(locations[loc]);
> > +               } else { /* native/static memory mode */
> >                         size = ndctl_namespace_get_size(ndns);
> > -               jobj = json_object_new_string("fsdax");
> > +                       jobj = json_object_new_string("mem");
> 
> We could just set NDCTL_PFN_LOC_RAM here.
> 
> > +               }
> > +               if (jobj)
> > +                       json_object_object_add(jndns, "map", jobj);
> >                 break;
> >         case NDCTL_NS_MODE_DAX:
> >                 if (!dax)
> >                         goto err;
> >                 size = ndctl_dax_get_size(dax);
> >                 jobj = json_object_new_string("devdax");
> > +               if (jobj)
> > +                       json_object_object_add(jndns, "mode", jobj);
> > +               loc = ndctl_dax_get_location(dax);
> > +               jobj = json_object_new_string(locations[loc]);
> > +               if (jobj)
> > +                       json_object_object_add(jndns, "map", jobj);
> >                 break;
> >         case NDCTL_NS_MODE_SAFE:
> >                 if (!btt)
> >                         goto err;
> >                 jobj = json_object_new_string("sector");
> > +               if (jobj)
> > +                       json_object_object_add(jndns, "mode", jobj);
> >                 size = ndctl_btt_get_size(btt);
> >                 break;
> >         case NDCTL_NS_MODE_RAW:
> >                 size = ndctl_namespace_get_size(ndns);
> >                 jobj = json_object_new_string("raw");
> > +               if (jobj)
> > +                       json_object_object_add(jndns, "mode", jobj);
> >                 break;
> >         default:
> >                 jobj = NULL;
> >         }
> > -       if (jobj)
> > -               json_object_object_add(jndns, "mode", jobj);
> 
> Why is mode getting moved into each case?
> 
> I'd put the
> 
>     jobj = json_object_new_string(locations[loc]);
>     if (jobj)
>         json_object_object_add(jndns, "map", jobj);
> 
> ...at the end here and just gate it on mode != NDCTL_NS_MODE_SAFE &&
> mode != NDCTL_NS_MODE_RAW

Good points, this makes it much cleaner. I'll send a new version.
diff mbox

Patch

diff --git a/util/json.c b/util/json.c
index c606e1c..b020300 100644
--- a/util/json.c
+++ b/util/json.c
@@ -667,11 +667,17 @@  struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 {
 	struct json_object *jndns = json_object_new_object();
 	struct json_object *jobj, *jbbs = NULL;
+	const char *locations[] = {
+		[NDCTL_PFN_LOC_NONE] = "none",
+		[NDCTL_PFN_LOC_RAM] = "mem",
+		[NDCTL_PFN_LOC_PMEM] = "dev",
+	};
 	unsigned long long size = ULLONG_MAX;
 	unsigned int sector_size = UINT_MAX;
 	enum ndctl_namespace_mode mode;
 	const char *bdev = NULL, *name;
 	unsigned int bb_count = 0;
+	enum ndctl_pfn_loc loc;
 	struct ndctl_btt *btt;
 	struct ndctl_pfn *pfn;
 	struct ndctl_dax *dax;
@@ -693,33 +699,49 @@  struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 	mode = ndctl_namespace_get_mode(ndns);
 	switch (mode) {
 	case NDCTL_NS_MODE_MEMORY:
-		if (pfn) /* dynamic memory mode */
+		jobj = json_object_new_string("fsdax");
+		if (jobj)
+			json_object_object_add(jndns, "mode", jobj);
+		loc = ndctl_pfn_get_location(pfn);
+		if (pfn) { /* dynamic memory mode */
 			size = ndctl_pfn_get_size(pfn);
-		else /* native/static memory mode */
+			jobj = json_object_new_string(locations[loc]);
+		} else { /* native/static memory mode */
 			size = ndctl_namespace_get_size(ndns);
-		jobj = json_object_new_string("fsdax");
+			jobj = json_object_new_string("mem");
+		}
+		if (jobj)
+			json_object_object_add(jndns, "map", jobj);
 		break;
 	case NDCTL_NS_MODE_DAX:
 		if (!dax)
 			goto err;
 		size = ndctl_dax_get_size(dax);
 		jobj = json_object_new_string("devdax");
+		if (jobj)
+			json_object_object_add(jndns, "mode", jobj);
+		loc = ndctl_dax_get_location(dax);
+		jobj = json_object_new_string(locations[loc]);
+		if (jobj)
+			json_object_object_add(jndns, "map", jobj);
 		break;
 	case NDCTL_NS_MODE_SAFE:
 		if (!btt)
 			goto err;
 		jobj = json_object_new_string("sector");
+		if (jobj)
+			json_object_object_add(jndns, "mode", jobj);
 		size = ndctl_btt_get_size(btt);
 		break;
 	case NDCTL_NS_MODE_RAW:
 		size = ndctl_namespace_get_size(ndns);
 		jobj = json_object_new_string("raw");
+		if (jobj)
+			json_object_object_add(jndns, "mode", jobj);
 		break;
 	default:
 		jobj = NULL;
 	}
-	if (jobj)
-		json_object_object_add(jndns, "mode", jobj);
 
 	if (size < ULLONG_MAX) {
 		jobj = util_json_object_size(size, flags);