[PATCH V2 nvme-cli 1/2] initialize disk "access" variable

[Chaitanya Kulkarni]

On 9/26/21 6:41 PM, chengjike wrote:
> External email: Use caution opening links or attachments
> 
> 
> When it is failed to get nvme_id_ns data from NVMe Subsystem, the failure may be caused
> by a link disconnection or an error code returned by the subsystem(such as NVME_SC_*).
> Then the "access" of disk is set to "faulty"(otherwise, set the value to "live").
> Some displaying content of fault device can be obtained from the disk attribute files.
> 

commit log is overly long and somewhat needs an improvement about what 
code changes we are doing... see below comments...

> Signed-off-by: chengjike <chengjike.cheng at huawei.com>
> ---
>   src/nvme/private.h |  1 +
>   src/nvme/tree.c    | 57 ++++++++++++++++++++++++++++++++++++++++++----
>   src/nvme/tree.h    |  8 +++++++
>   3 files changed, 61 insertions(+), 5 deletions(-)
> 
> diff --git a/src/nvme/private.h b/src/nvme/private.h
> index 2a151bf..255c1b1 100644
> --- a/src/nvme/private.h
> +++ b/src/nvme/private.h
> @@ -37,6 +37,7 @@ struct nvme_ns {
>          __u32 nsid;
>          char *name;
>          char *sysfs_dir;
> +       char *access;
> 
>          int lba_shift;
>          int lba_size;
> diff --git a/src/nvme/tree.c b/src/nvme/tree.c
> index 293b0c0..dea0f45 100644
> --- a/src/nvme/tree.c
> +++ b/src/nvme/tree.c
> @@ -229,9 +229,11 @@ nvme_ns_t nvme_subsystem_next_ns(nvme_subsystem_t s, nvme_ns_t n)
>   static void __nvme_free_ns(struct nvme_ns *n)
>   {
>          list_del_init(&n->entry);
> -       close(n->fd);
> +       if (n->fd > 0)
> +               close(n->fd);
>          free(n->name);
>          free(n->sysfs_dir);
> +       free(n->access);
>          free(n);
>   }
> 
> @@ -1023,9 +1025,6 @@ static int nvme_configure_ctrl(nvme_ctrl_t c, const char *path,
>          closedir(d);
> 
>          c->fd = nvme_open(name);
> -       if (c->fd < 0)
> -               return c->fd;
> -
>          c->name = strdup(name);
>          c->sysfs_dir = (char *)path;
>          c->firmware = nvme_get_ctrl_attr(c, "firmware_rev");
> @@ -1277,6 +1276,11 @@ const char *nvme_ns_get_name(nvme_ns_t n)
>          return n->name;
>   }
> 
> +const char *nvme_ns_get_access(nvme_ns_t n)
> +{
> +       return n->access;
> +}
> +

why introduce a function that is not used in this patch ? Is this a prep 
patch if so please update the commit log accordingly.

>   const char *nvme_ns_get_model(nvme_ns_t n)
>   {
>          return n->c ? n->c->model : n->s->model;
> @@ -1515,6 +1519,37 @@ free_ns:
>          return NULL;
>   }
> 
> +static nvme_ns_t nvme_ns_get_local_data(const char *name, const char *path)
> +{
> +       struct nvme_ns *n;
> +       char *ns_id;
> +
> +       n = calloc(1, sizeof(*n));
> +       if (!n) {
> +               errno = ENOMEM;
> +               return NULL;
> +       }
> +       memset(n, 0, sizeof(struct nvme_ns));
> +

setting the memory to zero when allocating from calloc()  ?

> +       n->name = strdup(name);
> +       n->fd = -1;
> +       ns_id = nvme_get_attr(path, "nsid");
> +       if (!ns_id)
> +               goto free_ns;

this label is used only once just return here and remove the goto label.

> +       n->nsid = atoi(ns_id);
> +       free(ns_id);
> +
> +       list_head_init(&n->paths);
> +       list_node_init(&n->entry);
> +
> +       return n;
> +
> +free_ns:
> +       free(n->name);
> +       free(n);
> +       return NULL;

above error handling code should be moved to if (!ns_id) ...

> +}
> +
>   static struct nvme_ns *__nvme_scan_namespace(const char *sysfs_dir, const char *name)
>   {
>          struct nvme_ns *n;
> @@ -1528,9 +1563,21 @@ static struct nvme_ns *__nvme_scan_namespace(const char *sysfs_dir, const char *
>          }
> 
>          n = nvme_ns_open(name);
> -       if (!n)
> +       if (!n) {
> +               n = nvme_ns_get_local_data(name, path);
> +               if (!n)
> +                       goto free_path;
> +               ret = asprintf(&n->access, "%s", "faulty");
> +               if (ret < 0)
> +                       goto free_path;
> +               goto get_attr;
> +       }
> +
> +       ret = asprintf(&n->access, "%s", "live");
> +       if (ret < 0)
>                  goto free_path;
> 
> +get_attr:
>          n->sysfs_dir = path;
>          return n;
> 
> diff --git a/src/nvme/tree.h b/src/nvme/tree.h
> index 68f5cbf..7d3d049 100644
> --- a/src/nvme/tree.h
> +++ b/src/nvme/tree.h
> @@ -452,6 +452,14 @@ const char *nvme_ns_get_sysfs_dir(nvme_ns_t n);
>    */
>   const char *nvme_ns_get_name(nvme_ns_t n);
> 
> +/**
> + * nvme_ns_get_access() -
> + * @n:
> + *
> + * Return:
> + */

Wrong function documentation comment, you need to be more descriptive
wht the function is doing...

> +const char *nvme_ns_get_access(nvme_ns_t n);
> +
>   /**
>    * nvme_ns_get_firmware() -
>    * @n:
> --
> 2.21.0.windows.1
>