| Message ID | 20170911113900.17E6.E1E9C6FF@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1cd52627c4b8 |
| Headers | show |
On Sun, Sep 10, 2017 at 7:39 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: >> > + >> > + if (!found) >> > + goto err_found; >> > + >> > + for (i = 0; i < found; i++) { >> > + const char *devname = ndctl_dimm_get_devname(dimms[i]); >> > + >> > + jobj = json_object_new_string(devname); >> > + if (!jobj) >> > + break; >> > + json_object_array_add(jdimms, jobj); >> > + } >> >> I have one comment. >> >> If output is sorted by device number here, >> it may be friendly for users. >> (Especially, # of interleave way becomes huge in future...) >> >> Other things are looks good for me. > > I made a patch to make sorting. > I hope it will help.... > > ------- > To make user friendly, it is desirable that ndctl sorts by dimm's number, > when output dimms which have badblocks. > > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com> > > --- > util/json.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/util/json.c b/util/json.c > index 9b7773e..dac8692 100644 > --- a/util/json.c > +++ b/util/json.c > @@ -366,6 +366,21 @@ struct json_object *util_daxctl_region_to_json(struct daxctl_region *region, > return NULL; > } > > +static int compare_dimm_number(const void *p1, const void *p2) > +{ > + struct ndctl_dimm *dimm1 = *(struct ndctl_dimm **)p1; > + struct ndctl_dimm *dimm2 = *(struct ndctl_dimm **)p2; > + const char *dimm1_name = ndctl_dimm_get_devname(dimm1); > + const char *dimm2_name = ndctl_dimm_get_devname(dimm2); > + int num1, num2; > + > + sscanf(dimm1_name, "nmem%d", &num1); > + sscanf(dimm2_name, "nmem%d", &num2); > + > + return num1 - num2; > + > +} > + > static struct json_object *badblocks_to_jdimms(struct ndctl_region *region, > unsigned long long addr, unsigned long len) > { > @@ -399,6 +414,8 @@ static struct json_object *badblocks_to_jdimms(struct ndctl_region *region, > if (!found) > goto err_found; > > + qsort(dimms, found, sizeof(dimm), compare_dimm_number); > + > for (i = 0; i < found; i++) { > const char *devname = ndctl_dimm_get_devname(dimms[i]); Looks good to me.
> On Sun, Sep 10, 2017 at 7:39 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: > >> > + > >> > + if (!found) > >> > + goto err_found; > >> > + > >> > + for (i = 0; i < found; i++) { > >> > + const char *devname = ndctl_dimm_get_devname(dimms[i]); > >> > + > >> > + jobj = json_object_new_string(devname); > >> > + if (!jobj) > >> > + break; > >> > + json_object_array_add(jdimms, jobj); > >> > + } > >> > >> I have one comment. > >> > >> If output is sorted by device number here, > >> it may be friendly for users. > >> (Especially, # of interleave way becomes huge in future...) > >> > >> Other things are looks good for me. > > > > I made a patch to make sorting. > > I hope it will help.... > > > > ------- > > To make user friendly, it is desirable that ndctl sorts by dimm's number, > > when output dimms which have badblocks. > > > > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com> > > > > --- > > util/json.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/util/json.c b/util/json.c > > index 9b7773e..dac8692 100644 > > --- a/util/json.c > > +++ b/util/json.c > > @@ -366,6 +366,21 @@ struct json_object *util_daxctl_region_to_json(struct daxctl_region *region, > > return NULL; > > } > > > > +static int compare_dimm_number(const void *p1, const void *p2) > > +{ > > + struct ndctl_dimm *dimm1 = *(struct ndctl_dimm **)p1; > > + struct ndctl_dimm *dimm2 = *(struct ndctl_dimm **)p2; > > + const char *dimm1_name = ndctl_dimm_get_devname(dimm1); > > + const char *dimm2_name = ndctl_dimm_get_devname(dimm2); > > + int num1, num2; > > + > > + sscanf(dimm1_name, "nmem%d", &num1); > > + sscanf(dimm2_name, "nmem%d", &num2); > > + > > + return num1 - num2; > > + > > +} > > + > > static struct json_object *badblocks_to_jdimms(struct ndctl_region *region, > > unsigned long long addr, unsigned long len) > > { > > @@ -399,6 +414,8 @@ static struct json_object *badblocks_to_jdimms(struct ndctl_region *region, > > if (!found) > > goto err_found; > > > > + qsort(dimms, found, sizeof(dimm), compare_dimm_number); > > + > > for (i = 0; i < found; i++) { > > const char *devname = ndctl_dimm_get_devname(dimms[i]); > > Looks good to me. I confirmed the most of the patches (and some fixes by you) are includedin v58. (Thanks a lot!) But, I noticed that this sorting patch has been not merged yet. Could you merge this one too? Bye,
On Tue, Sep 12, 2017 at 11:34 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: >> On Sun, Sep 10, 2017 at 7:39 PM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote: >> >> > + >> >> > + if (!found) >> >> > + goto err_found; >> >> > + >> >> > + for (i = 0; i < found; i++) { >> >> > + const char *devname = ndctl_dimm_get_devname(dimms[i]); >> >> > + >> >> > + jobj = json_object_new_string(devname); >> >> > + if (!jobj) >> >> > + break; >> >> > + json_object_array_add(jdimms, jobj); >> >> > + } >> >> >> >> I have one comment. >> >> >> >> If output is sorted by device number here, >> >> it may be friendly for users. >> >> (Especially, # of interleave way becomes huge in future...) >> >> >> >> Other things are looks good for me. >> > >> > I made a patch to make sorting. >> > I hope it will help.... >> > >> > ------- >> > To make user friendly, it is desirable that ndctl sorts by dimm's number, >> > when output dimms which have badblocks. >> > >> > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com> >> > >> > --- >> > util/json.c | 17 +++++++++++++++++ >> > 1 file changed, 17 insertions(+) >> > >> > diff --git a/util/json.c b/util/json.c >> > index 9b7773e..dac8692 100644 >> > --- a/util/json.c >> > +++ b/util/json.c >> > @@ -366,6 +366,21 @@ struct json_object *util_daxctl_region_to_json(struct daxctl_region *region, >> > return NULL; >> > } >> > >> > +static int compare_dimm_number(const void *p1, const void *p2) >> > +{ >> > + struct ndctl_dimm *dimm1 = *(struct ndctl_dimm **)p1; >> > + struct ndctl_dimm *dimm2 = *(struct ndctl_dimm **)p2; >> > + const char *dimm1_name = ndctl_dimm_get_devname(dimm1); >> > + const char *dimm2_name = ndctl_dimm_get_devname(dimm2); >> > + int num1, num2; >> > + >> > + sscanf(dimm1_name, "nmem%d", &num1); >> > + sscanf(dimm2_name, "nmem%d", &num2); >> > + >> > + return num1 - num2; >> > + >> > +} >> > + >> > static struct json_object *badblocks_to_jdimms(struct ndctl_region *region, >> > unsigned long long addr, unsigned long len) >> > { >> > @@ -399,6 +414,8 @@ static struct json_object *badblocks_to_jdimms(struct ndctl_region *region, >> > if (!found) >> > goto err_found; >> > >> > + qsort(dimms, found, sizeof(dimm), compare_dimm_number); >> > + >> > for (i = 0; i < found; i++) { >> > const char *devname = ndctl_dimm_get_devname(dimms[i]); >> >> Looks good to me. > > I confirmed the most of the patches (and some fixes by you) are > includedin v58. (Thanks a lot!) > But, I noticed that this sorting patch has been not merged yet. > > Could you merge this one too? Applied, and I'll include it in a v58.2 release that I have staged to fix up so other v58 regressions.
diff --git a/util/json.c b/util/json.c index 9b7773e..dac8692 100644 --- a/util/json.c +++ b/util/json.c @@ -366,6 +366,21 @@ struct json_object *util_daxctl_region_to_json(struct daxctl_region *region, return NULL; } +static int compare_dimm_number(const void *p1, const void *p2) +{ + struct ndctl_dimm *dimm1 = *(struct ndctl_dimm **)p1; + struct ndctl_dimm *dimm2 = *(struct ndctl_dimm **)p2; + const char *dimm1_name = ndctl_dimm_get_devname(dimm1); + const char *dimm2_name = ndctl_dimm_get_devname(dimm2); + int num1, num2; + + sscanf(dimm1_name, "nmem%d", &num1); + sscanf(dimm2_name, "nmem%d", &num2); + + return num1 - num2; + +} + static struct json_object *badblocks_to_jdimms(struct ndctl_region *region, unsigned long long addr, unsigned long len) { @@ -399,6 +414,8 @@ static struct json_object *badblocks_to_jdimms(struct ndctl_region *region, if (!found) goto err_found; + qsort(dimms, found, sizeof(dimm), compare_dimm_number); + for (i = 0; i < found; i++) { const char *devname = ndctl_dimm_get_devname(dimms[i]);