Re: [ndctl PATCH 4/8] ndctl/namespace: Add read-info-block command

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