Messages in this thread |  | | Date | Sun, 25 Dec 2022 12:36:07 +0200 | Subject | Re: drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret' | From | Sagi Grimberg <> |
| |
On 12/23/22 17:47, Christoph Hellwig wrote: > Based on the code in nvme_auth_generate_key I assume this is intentional, > but the code looks really confusing. > > Hannes, Sagi, what do you think of something like this: > > > diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c > index d90e4f0c08b7b9..a07eb4cd9ce173 100644 > --- a/drivers/nvme/common/auth.c > +++ b/drivers/nvme/common/auth.c > @@ -455,28 +455,18 @@ int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm, > } > EXPORT_SYMBOL_GPL(nvme_auth_gen_shared_secret); > > -int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key) > +struct nvme_dhchap_key *nvme_auth_generate_key(u8 *secret) > { > - struct nvme_dhchap_key *key; > u8 key_hash; > > - if (!secret) { > - *ret_key = NULL; > - return 0; > - } > + if (!secret) > + return NULL; > > if (sscanf(secret, "DHHC-1:%hhd:%*s:", &key_hash) != 1) > - return -EINVAL; > + return ERR_PTR(-EINVAL); > > /* Pass in the secret without the 'DHHC-1:XX:' prefix */ > - key = nvme_auth_extract_key(secret + 10, key_hash); > - if (IS_ERR(key)) { > - *ret_key = NULL; > - return PTR_ERR(key); > - } > - > - *ret_key = key; > - return 0; > + return nvme_auth_extract_key(secret + 10, key_hash); > } > EXPORT_SYMBOL_GPL(nvme_auth_generate_key); > > diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c > index bb0abbe4491cdc..c808652966a94f 100644 > --- a/drivers/nvme/host/auth.c > +++ b/drivers/nvme/host/auth.c > @@ -943,16 +943,19 @@ int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl) > INIT_WORK(&ctrl->dhchap_auth_work, nvme_ctrl_auth_work); > if (!ctrl->opts) > return 0; > - ret = nvme_auth_generate_key(ctrl->opts->dhchap_secret, > - &ctrl->host_key); > - if (ret) > - return ret; > - ret = nvme_auth_generate_key(ctrl->opts->dhchap_ctrl_secret, > - &ctrl->ctrl_key); > - if (ret) > + > + ctrl->host_key = nvme_auth_generate_key(ctrl->opts->dhchap_secret); > + if (IS_ERR(ctrl->host_key)) {
nvme_auth_generate_key can return NULL, so in this case we should avoid calling it if the secret is null here.
Other than that, this looks good. Although I think that for this specific report, we should do a simple fix and then make the code look better.
|  |