linux-nvdimm.lists.01.org archive mirror
 help / color / mirror / Atom feed
From: "Verma, Vishal L" <vishal.l.verma@intel.com>
To: "Williams, Dan J" <dan.j.williams@intel.com>,
	"linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>
Subject: Re: [ndctl PATCH 4/8] ndctl/namespace: Add read-info-block command
Date: Tue, 19 Mar 2019 23:11:28 +0000	[thread overview]
Message-ID: <544793f37222f7cbf50209f96eabbc7f5eb79eae.camel@intel.com> (raw)
In-Reply-To: <155000694931.348282.10012437535725994854.stgit@dwillia2-desk3.amr.corp.intel.com>


On Tue, 2019-02-12 at 13:29 -0800, Dan Williams wrote:
> Namespaces may contain an info-block that correlates with the possible
> modes of a namespace: 'sector', 'fsdax', or 'devdax'. Provide a command
> that can temporarily convert the namespace into 'raw' mode to read the
> info-block.
> 
> Also, similar to 'read-labels' provide a facility to decode the
> info-block into json.

Nice, I like this!

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  ndctl/action.h    |    1 
>  ndctl/builtin.h   |    1 
>  ndctl/check.c     |   20 ---
>  ndctl/namespace.c |  401 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ndctl/namespace.h |   51 +++++++
>  ndctl/ndctl.c     |    1 
>  util/fletcher.h   |    1 
>  util/size.h       |    1 
>  8 files changed, 456 insertions(+), 21 deletions(-)

[..]

>  
> +#define READ_INFOBLOCK_OPTIONS() \
> +OPT_FILENAME('o', "output", &param.outfile, "output-file", \
> +	"filename to write label area contents"), \

copy/paste staleness, should be "write infoblock contents"

> +OPT_FILENAME('f', "file", &param.infile, "input-file", \
> +	"filename to readinfo block instead of a namespace"), \

Perhaps make this consistent with the label commands, and call the file
option -i/--input. This also frees up -f for --force, which might be
nice to have since the namespaces are expected to be disabled (That's
only a minor convenience thing though - I'm equally OK leaving out  the
--force for now)

> +OPT_BOOLEAN('n', "no-verify", &param.no_verify, \
> +	"skip parent uuid, and checksum validation"), \

Since the option handling framework automatically provides a no-* option
for boolean options, should we call this --verify, default to on, and
then document the availability of the automatic --no-verify option?

Currently, this ends up in an automatic --no-no-verify option :)

> +OPT_BOOLEAN('j', "json", &param.json, "parse label data into json"), \
> +OPT_BOOLEAN('u', "human", &param.human, "use human friendly number formats ")
> +

[..]

> +	if (mode == ACTION_READ_INFOBLOCK && param.infile && argc) {
> +		error("specify a namespace, or --file, not both\n");
> +		rc = -EINVAL;
> +	}
> +

Similar comment as read-labels about auto enabling --json if --human
supplied standalone.



_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

  reply	other threads:[~2019-03-19 23:11 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-12 21:28 [ndctl PATCH 0/8] Improve support + testing for labels + info-blocks Dan Williams
2019-02-12 21:28 ` [ndctl PATCH 1/8] ndctl/dimm: Add 'flags' field to read-labels output Dan Williams
2019-02-12 21:28 ` [ndctl PATCH 2/8] ndctl/dimm: Add --human support to read-labels Dan Williams
2019-03-19 22:39   ` Verma, Vishal L
2019-03-19 22:51     ` Dan Williams
2019-02-12 21:29 ` [ndctl PATCH 3/8] ndctl/build: Drop -Wpointer-arith Dan Williams
2019-02-12 21:29 ` [ndctl PATCH 4/8] ndctl/namespace: Add read-info-block command Dan Williams
2019-03-19 23:11   ` Verma, Vishal L [this message]
2019-02-12 21:29 ` [ndctl PATCH 5/8] ndctl/test: Update dax-dev to handle multiple e820 ranges Dan Williams
2019-02-12 21:29 ` [ndctl PATCH 6/8] ndctl/test: Make dax.sh more robust vs small namespaces Dan Williams
2019-02-12 21:29 ` [ndctl PATCH 7/8] ndctl/namespace: Always zero info-blocks Dan Williams
2019-02-12 21:29 ` [ndctl PATCH 8/8] ndctl/test: Test inter-region collision handling Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=544793f37222f7cbf50209f96eabbc7f5eb79eae.camel@intel.com \
    --to=vishal.l.verma@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=linux-nvdimm@lists.01.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).