From mboxrd@z Thu Jan 1 00:00:00 1970 From: minwoo.im.dev@gmail.com (Minwoo Im) Date: Thu, 25 Jul 2019 22:37:36 +0900 Subject: [PATCH 4/5] lnvm: introduce chunk-log command for chunk info In-Reply-To: <20B9C888-532C-4762-9886-8C5E196CB5A4@javigon.com> References: <20190723183601.29370-1-minwoo.im.dev@gmail.com> <20190723183601.29370-5-minwoo.im.dev@gmail.com> <20B9C888-532C-4762-9886-8C5E196CB5A4@javigon.com> Message-ID: <20190725133736.GB3018@minwoo-desktop> Hi Javier, Thanks for the review. > > +int lnvm_do_chunk_log(int fd, __u32 nsid, __u32 data_len, void *data, > > + unsigned int flags) > > +{ > > + int err; > > + > > + err = nvme_get_log13(fd, nsid, NVM_LID_CHUNK_INFO, 0, 0, 0, > > + false, data_len, data); > > + if (!err) { > > + if (flags & RAW) > > + d_raw(data, data_len); > > + else > > + show_lnvm_chunk_log(data, data_len); > > + } else if (err > 0) > > + fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n", > > + nvme_status_to_string(err), err, nsid); > > + return err; > > What about avoiding the nested if? > > If (err) { > fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n", > nvme_status_to_string(err), err, nsid); > > goto out; > } > > If (flags & RAW) > d_raw(data, data_len); > else > show_lnvm_chunk_log(data, data_len); > > out: > return err; That makes sense, we should avoid the case when "err" is negative value which means the internal error from the nvme_get_log13() in printing the nvme status. So how about this? ```c if (err > 0) { fprintf(stderr, "NVMe Status:%s(%x) NSID:%d\n", nvme_status_to_string(err), err, nsid); goto out; } else if (err < 0) { err = -errno; perror("nvme_get_log13"); goto out; } if (flags & RAW) d_raw(data, data_len); else show_lnvm_chunk_log(data, data_len); out: return err; ``` > > +} > > + > > static void show_lnvm_bbtbl(struct nvme_nvm_bb_tbl *tbl) > > { > > printf("verid : %#x\n", (uint16_t)le16_to_cpu(tbl->verid)); > > diff --git a/nvme-lightnvm.h b/nvme-lightnvm.h > > index 7a8ef7d..19660b7 100644 > > --- a/nvme-lightnvm.h > > +++ b/nvme-lightnvm.h > > @@ -322,6 +322,7 @@ int lnvm_do_create_tgt(char *, char *, char *, int, int, int, int); > > int lnvm_do_remove_tgt(char *); > > int lnvm_do_factory_init(char *, int, int, int); > > int lnvm_do_id_ns(int, int, unsigned int); > > +int lnvm_do_chunk_log(int, __u32, __u32, void *, unsigned int); > > int lnvm_do_get_bbtbl(int, int, int, int, unsigned int); > > int lnvm_do_set_bbtbl(int, int, int, int, int, int, __u8); > > > > diff --git a/plugins/lnvm/lnvm-nvme.c b/plugins/lnvm/lnvm-nvme.c > > index 754931a..6f2724a 100644 > > --- a/plugins/lnvm/lnvm-nvme.c > > +++ b/plugins/lnvm/lnvm-nvme.c > > @@ -1,5 +1,7 @@ > > #include > > #include > > +#include > > +#include > > > > #include "nvme.h" > > #include "nvme-print.h" > > @@ -127,6 +129,88 @@ static int lnvm_id_ns(int argc, char **argv, struct command *cmd, struct plugin > > return lnvm_do_id_ns(fd, cfg.namespace_id, flags); > > } > > > > +static int lnvm_chunk_log(int argc, char **argv, struct command *cmd, struct plugin *plugin) > > +{ > > + const char *desc = "Retrieve the chunk information log for the "\ > > + "specified given LightNVM device, returns in either "\ > > + "human-readable or binary format.\n"\ > > + "This will request Geometry first to get the "\ > > + "num_grp,num_pu,num_chk first to figure out the total size "\ > > + "of the log pages."\ > > + ; > > + const char *output_format = "Output format: normal|binary?; > > normalbinary -> normal binary? Oh It has "|" between them. But I'm fine with that too. It was just derieved from the nvme.c core module which builtin commands are taking now. > > + const char *human_readable = "Print normal in readable format"; > > + int err, fmt, fd; > > + struct nvme_nvm_id20 geo; > > + struct nvme_nvm_chunk_desc *chunk_log; > > + __u32 nsid; > > + __u32 data_len; > > + unsigned int flags = 0; > > + > > + struct config { > > + char *output_format; > > + int human_readable; > > + }; > > + > > + struct config cfg = { > > + .output_format = "normal", > > + }; > > + > > + const struct argconfig_commandline_options command_line_options[] = { > > + {"output-format", 'o', "FMT", CFG_STRING, &cfg.output_format, required_argument, output_format}, > > + {"human-readable",'H', "", CFG_NONE, &cfg.human_readable,no_argument, human_readable}, > > + {NULL} > > + }; > > + > > + fd = parse_and_open(argc, argv, desc, command_line_options, &cfg, > > + sizeof(cfg)); > > + if (fd < 0) { > > + err = fd; > > + goto ret; > > + } > > return fd; This is absolutely no need to set the "err" value in this step. Will update. > > > + > > + fmt = validate_output_format(cfg.output_format); > > + if (fmt < 0) { > > + err = fmt; > > + goto close; > > + } > > + > > + if (fmt == BINARY) > > + flags |= RAW; > > + else if (cfg.human_readable) > > + flags |= HUMAN; > > + > > + nsid = nvme_get_nsid(fd); > > + > > + /* > > + * It needs to figure out how many bytes will be requested by this > > + * subcommand by the (num_grp * num_pu * num_chk) from the Geometry. > > + */ > > + err = lnvm_get_identity(fd, nsid, (struct nvme_nvm_id *) &geo); > > + if (err) > > + goto close; > > + > > + data_len = (geo.num_grp * geo.num_pu * geo.num_chk) * > > + sizeof(struct nvme_nvm_chunk_desc); > > + chunk_log = malloc(data_len); > > + if (!chunk_log) { > > + fprintf(stderr, "cound not alloc for chunk log %dbytes\n", > > + data_len); > > + err = -ENOMEM; > > + goto close; > > + } > > + > > + err = lnvm_do_chunk_log(fd, nsid, data_len, chunk_log, flags); > > + if (err) > > + fprintf(stderr, "get log page for chunk information failed\n"); > > + > > + free(chunk_log); > > +close: > > + close(fd); > > +ret: > > + return err; > > +} > > + > > static int lnvm_create_tgt(int argc, char **argv, struct command *cmd, struct plugin *plugin) > > { > > const char *desc = "Instantiate a target on top of a LightNVM enabled device."; > > diff --git a/plugins/lnvm/lnvm-nvme.h b/plugins/lnvm/lnvm-nvme.h > > index 3d5cbc5..f091f7b 100644 > > --- a/plugins/lnvm/lnvm-nvme.h > > +++ b/plugins/lnvm/lnvm-nvme.h > > @@ -12,6 +12,7 @@ PLUGIN(NAME("lnvm", "LightNVM specific extensions"), > > ENTRY("list", "List available LightNVM devices", lnvm_list) > > ENTRY("info", "List general information and available target engines", lnvm_info) > > ENTRY("id-ns", "List geometry for LightNVM device", lnvm_id_ns) > > + ENTRY("chunk-log", "Chunk information by Get Log Page", lnvm_chunk_log) > > ENTRY("init", "Initialize media manager on LightNVM device", lnvm_init) > > ENTRY("create", "Create target on top of a LightNVM device", lnvm_create_tgt) > > ENTRY("remove", "Remove target from device", lnvm_remove_tgt) > > -- > > 2.17.1 > > We have typically retrieved this directly using passthru commands, but I > have nothing against making this a dedicated lnvm command. Thanks for letting me know. I was just trying to provide a dedicated command just like the built-in commands like fw-log and etc. Thanks for the all these kindly review! I will prepare the next version with these points.