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

Chengjike (ISSP) chengjike.cheng at huawei.com
Tue Sep 28 01:49:02 PDT 2021


在 2021/9/28 3:34, 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
>>
Hi, Chaitanya

Thank you for your comments very much.
This patch is a prep patch, and the "nvme_ns_get_access" function is 
used in the next patch.
In addition, I will resend patch V3 based on your comments.






More information about the Linux-nvme mailing list