[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