Re: [PATCH 1/3] nvme: add generic helper to store secret

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 5/17/23 00:31, Sagi Grimberg wrote:
>
>
> On 5/16/23 13:06, Chaitanya Kulkarni wrote:
>> Refactor code to avoid duplication and improve maintainability:
>>
>> Consolidate the shared code between the functions
>> nvme_ctrl_dhchap_secret_store() and
>> nvme_ctrl_dhchap_ctrl_secret_store(). This duplication not only
>> increases the likelihood of bugs but also requires additional effort for
>> maintenance and testing.
>>
>> Introduce a new generic helper function called
>> nvme_dhchap_secret_store_common() to handle the storage of the
>> dhchap secret. This helper function will be used by both
>> nvme_ctrl_dhchap_secret_store() and
>> nvme_ctrl_dhchap_ctrl_secret_store().
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/nvme/host/sysfs.c | 59 ++++++++++++++++++++++++++-------------
>>   1 file changed, 39 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index 796e1d373b7c..9ce3b16f06da 100644
>> --- a/drivers/nvme/host/sysfs.c
>> +++ b/drivers/nvme/host/sysfs.c
>> @@ -418,43 +418,53 @@ static ssize_t 
>> nvme_ctrl_dhchap_secret_show(struct device *dev,
>>       return sysfs_emit(buf, "%s\n", opts->dhchap_secret);
>>   }
>>   -static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
>> -        struct device_attribute *attr, const char *buf, size_t count)
>> +static ssize_t nvme_dhchap_secret_store_common(struct nvme_ctrl *ctrl,
>> +        const char *buf, size_t count, bool ctrl_secret)
>>   {
>> -    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> -    struct nvmf_ctrl_options *opts = ctrl->opts;
>> -    char *dhchap_secret;
>> +    struct nvme_dhchap_key **orig_key;
>> +    char **dhchap_secret;
>> +    char *new_dhchap_secret;
>> +
>> +    if (ctrl_secret) {
>> +        if (!ctrl->opts->dhchap_ctrl_secret)
>> +            return -EINVAL;
>> +        dhchap_secret = &ctrl->opts->dhchap_ctrl_secret;
>> +        orig_key = &ctrl->ctrl_key;
>> +    } else {
>> +        if (!ctrl->opts->dhchap_secret)
>> +            return -EINVAL;
>> +        dhchap_secret = &ctrl->opts->dhchap_secret;
>> +        orig_key = &ctrl->host_key;
>> +    }
>>   -    if (!ctrl->opts->dhchap_secret)
>> -        return -EINVAL;
>>       if (count < 7)
>>           return -EINVAL;
>>       if (memcmp(buf, "DHHC-1:", 7))
>>           return -EINVAL;
>>   -    dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
>> -    if (!dhchap_secret)
>> +    new_dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
>> +    if (!new_dhchap_secret)
>>           return -ENOMEM;
>> -    memcpy(dhchap_secret, buf, count);
>> +    memcpy(new_dhchap_secret, buf, count);
>>       nvme_auth_stop(ctrl);
>> -    if (strcmp(dhchap_secret, opts->dhchap_secret)) {
>> -        struct nvme_dhchap_key *key, *host_key;
>> +    if (strcmp(new_dhchap_secret, *dhchap_secret)) {
>> +        struct nvme_dhchap_key *new_key, *prev_host_key;
>
> prev_host_key? or prev_key?


will fix it in the next version ..

-ck