backends/cryptodev-lkcf.c | 3 +++ 1 file changed, 3 insertions(+)
When cryptodev_lkcf_set_op_desc() fails, we report an error, but
continue anyway. This is wrong. We then pass a non-null @local_error
to various functions, which could easily fail error_setv()'s assertion
on failure.
Fail the function instead.
When qcrypto_akcipher_new() fails, we fail the function without
reporting the error. This leaks the Error object.
Add the missing error reporting. This also frees the Error object.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
backends/cryptodev-lkcf.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
index 41cf24b737..352c3e8958 100644
--- a/backends/cryptodev-lkcf.c
+++ b/backends/cryptodev-lkcf.c
@@ -330,6 +330,8 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
cryptodev_lkcf_set_op_desc(&session->akcipher_opts, op_desc,
sizeof(op_desc), &local_error) != 0) {
error_report_err(local_error);
+ status = -VIRTIO_CRYPTO_ERR;
+ goto out;
} else {
key_id = add_key(KCTL_KEY_TYPE_PKEY, "lkcf-backend-priv-key",
p8info, p8info_len, KCTL_KEY_RING);
@@ -346,6 +348,7 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
session->key, session->keylen,
&local_error);
if (!akcipher) {
+ error_report_err(local_error);
status = -VIRTIO_CRYPTO_ERR;
goto out;
}
--
2.48.1Queued for 10.0.
LGTM, thanks!
Reviewed-by: zhenwei pi <pizhenwei@bytedance.com>
On 3/12/25 18:11, Markus Armbruster wrote:
> When cryptodev_lkcf_set_op_desc() fails, we report an error, but
> continue anyway. This is wrong. We then pass a non-null @local_error
> to various functions, which could easily fail error_setv()'s assertion
> on failure.
>
> Fail the function instead.
>
> When qcrypto_akcipher_new() fails, we fail the function without
> reporting the error. This leaks the Error object.
>
> Add the missing error reporting. This also frees the Error object.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> backends/cryptodev-lkcf.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
> index 41cf24b737..352c3e8958 100644
> --- a/backends/cryptodev-lkcf.c
> +++ b/backends/cryptodev-lkcf.c
> @@ -330,6 +330,8 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
> cryptodev_lkcf_set_op_desc(&session->akcipher_opts, op_desc,
> sizeof(op_desc), &local_error) != 0) {
> error_report_err(local_error);
> + status = -VIRTIO_CRYPTO_ERR;
> + goto out;
> } else {
> key_id = add_key(KCTL_KEY_TYPE_PKEY, "lkcf-backend-priv-key",
> p8info, p8info_len, KCTL_KEY_RING);
> @@ -346,6 +348,7 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
> session->key, session->keylen,
> &local_error);
> if (!akcipher) {
> + error_report_err(local_error);
> status = -VIRTIO_CRYPTO_ERR;
> goto out;
> }
On 3/12/25 18:11, Markus Armbruster wrote:
> When cryptodev_lkcf_set_op_desc() fails, we report an error, but
> continue anyway. This is wrong. We then pass a non-null @local_error
> to various functions, which could easily fail error_setv()'s assertion
> on failure.
>
> Fail the function instead.
>
> When qcrypto_akcipher_new() fails, we fail the function without
> reporting the error. This leaks the Error object.
>
> Add the missing error reporting. This also frees the Error object.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> backends/cryptodev-lkcf.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
> index 41cf24b737..352c3e8958 100644
> --- a/backends/cryptodev-lkcf.c
> +++ b/backends/cryptodev-lkcf.c
> @@ -330,6 +330,8 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
> cryptodev_lkcf_set_op_desc(&session->akcipher_opts, op_desc,
> sizeof(op_desc), &local_error) != 0) {
> error_report_err(local_error);
> + status = -VIRTIO_CRYPTO_ERR;
> + goto out;
> } else {
> key_id = add_key(KCTL_KEY_TYPE_PKEY, "lkcf-backend-priv-key",
> p8info, p8info_len, KCTL_KEY_RING);
> @@ -346,6 +348,7 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
> session->key, session->keylen,
> &local_error);
> if (!akcipher) {
> + error_report_err(local_error);
> status = -VIRTIO_CRYPTO_ERR;
> goto out;
> }
What about moving several 'error_report_err(local_error);' to:
out:
if (local_error) {
error_report_err(local_error);
}
zhenwei pi <pizhenwei@bytedance.com> writes:
> On 3/12/25 18:11, Markus Armbruster wrote:
>> When cryptodev_lkcf_set_op_desc() fails, we report an error, but
>> continue anyway. This is wrong. We then pass a non-null @local_error
>> to various functions, which could easily fail error_setv()'s assertion
>> on failure.
>>
>> Fail the function instead.
>>
>> When qcrypto_akcipher_new() fails, we fail the function without
>> reporting the error. This leaks the Error object.
>>
>> Add the missing error reporting. This also frees the Error object.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> backends/cryptodev-lkcf.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
>> index 41cf24b737..352c3e8958 100644
>> --- a/backends/cryptodev-lkcf.c
>> +++ b/backends/cryptodev-lkcf.c
>> @@ -330,6 +330,8 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
>> cryptodev_lkcf_set_op_desc(&session->akcipher_opts, op_desc,
>> sizeof(op_desc), &local_error) != 0) {
>> error_report_err(local_error);
>> + status = -VIRTIO_CRYPTO_ERR;
>> + goto out;
>> } else {
>> key_id = add_key(KCTL_KEY_TYPE_PKEY, "lkcf-backend-priv-key",
>> p8info, p8info_len, KCTL_KEY_RING);
>> @@ -346,6 +348,7 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
>> session->key, session->keylen,
>> &local_error);
>> if (!akcipher) {
>> + error_report_err(local_error);
>> status = -VIRTIO_CRYPTO_ERR;
>> goto out;
>> }
>
> What about moving several 'error_report_err(local_error);' to:
>
> out:
> if (local_error) {
> error_report_err(local_error);
> }
I figure you suggest something like the appended patch. But this led me
to another question. Consider:
asym_op_info = task->op_info->u.asym_op_info;
switch (op_code) {
case VIRTIO_CRYPTO_AKCIPHER_ENCRYPT:
if (key_id >= 0) {
ret = keyctl_pkey_encrypt(key_id, op_desc,
asym_op_info->src, asym_op_info->src_len,
asym_op_info->dst, asym_op_info->dst_len);
When keyctl_pkey_encrypt() fails, @local_error remains unset.
} else {
ret = qcrypto_akcipher_encrypt(akcipher,
asym_op_info->src, asym_op_info->src_len,
asym_op_info->dst, asym_op_info->dst_len, &local_error);
}
break;
[More cases that can also fail without setting @local_error]
default:
error_setg(&local_error, "Unknown opcode: %u", op_code);
status = -VIRTIO_CRYPTO_ERR;
goto out;
}
if (ret < 0) {
The switch failed.
if (!local_error) {
If it failed without setting @local_error, we report a generic error
*unless* errno is EKEYREJECTED.
Aside: checking errno this far from whatever set it is asking for
trouble. It gets overwritten easily.
if (errno != EKEYREJECTED) {
error_report("Failed do operation with keyctl: %d", errno);
}
If it failed with setting @local_error, we report that error.
} else {
error_report_err(local_error);
}
status = op_code == VIRTIO_CRYPTO_AKCIPHER_VERIFY ?
-VIRTIO_CRYPTO_KEY_REJECTED : -VIRTIO_CRYPTO_ERR;
Status set to negative value. This will be assigned to task->status
below.
It can therefore become negative *silently* (i.e. without an error
report). Why is this okay?
} else {
status = VIRTIO_CRYPTO_OK;
asym_op_info->dst_len = ret;
}
out:
if (key_id >= 0) {
keyctl_unlink(key_id, KCTL_KEY_RING);
}
task->status = status;
diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
index 41cf24b737..0e20797cb3 100644
--- a/backends/cryptodev-lkcf.c
+++ b/backends/cryptodev-lkcf.c
@@ -329,7 +329,8 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
&local_error) != 0 ||
cryptodev_lkcf_set_op_desc(&session->akcipher_opts, op_desc,
sizeof(op_desc), &local_error) != 0) {
- error_report_err(local_error);
+ status = -VIRTIO_CRYPTO_ERR;
+ goto out;
} else {
key_id = add_key(KCTL_KEY_TYPE_PKEY, "lkcf-backend-priv-key",
p8info, p8info_len, KCTL_KEY_RING);
@@ -410,10 +411,9 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
if (ret < 0) {
if (!local_error) {
if (errno != EKEYREJECTED) {
- error_report("Failed do operation with keyctl: %d", errno);
+ error_setg_errno(&local_error,
+ "Failed do operation with keyctl: %d");
}
- } else {
- error_report_err(local_error);
}
status = op_code == VIRTIO_CRYPTO_AKCIPHER_VERIFY ?
-VIRTIO_CRYPTO_KEY_REJECTED : -VIRTIO_CRYPTO_ERR;
@@ -423,6 +423,9 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
}
out:
+ if (local_error) {
+ error_report_err(local_error);
+ }
if (key_id >= 0) {
keyctl_unlink(key_id, KCTL_KEY_RING);
}
Hi Markus,
Current code style seems buggy, I think the main reason is that the
Error *errp is not generated at right place. keyctl_pkey_XXX fails
without new error, qcrypto_akcipher_XXX fails with new error, but they
are in the same switch-case code block. If we can separate crypto
operations into two functions - cryptodev_lkcf_keyctl_op and
cryptodev_lkcf_qakcipher_op, and the error is generate inside the
functions, it may be handled easily. Then applying your changes, it seem
more clear. What do you think?
+static inline int cryptodev_lkcf_keyctl_op(key_serial_t key_id, int
op_code, char *op_desc,
+ CryptoDevBackendAsymOpInfo *asym_op_info, Error
**errp)
+{
+ int ret = -1;
+
+ switch (op_code) {
+ case VIRTIO_CRYPTO_AKCIPHER_ENCRYPT:
+ ret = keyctl_pkey_encrypt(key_id, op_desc,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_DECRYPT:
+ ret = keyctl_pkey_decrypt(key_id, op_desc,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_SIGN:
+ ret = keyctl_pkey_sign(key_id, op_desc,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_VERIFY:
+ ret = keyctl_pkey_verify(key_id, op_desc,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len);
+ break;
+
+ default:
+ error_setg(errp, "Unknown opcode: %u", op_code);
+ }
+
+ if (ret && !*errp) {
+ error_setg_errno(errp, errno, "Failed do operation with keyctl");
+ }
+
+ return ret;
+}
+
+static inline int cryptodev_lkcf_qakcipher_op(QCryptoAkCipher
*akcipher, int op_code, char *op_desc,
+ CryptoDevBackendAsymOpInfo *asym_op_info, Error
**errp)
+{
+ int ret = -1;
+
+ switch (op_code) {
+ case VIRTIO_CRYPTO_AKCIPHER_ENCRYPT:
+ ret = qcrypto_akcipher_encrypt(akcipher,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len, errp);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_DECRYPT:
+ ret = qcrypto_akcipher_decrypt(akcipher,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len, errp);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_SIGN:
+ ret = qcrypto_akcipher_sign(akcipher,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len, errp);
+ break;
+
+ case VIRTIO_CRYPTO_AKCIPHER_VERIFY:
+ ret = qcrypto_akcipher_verify(akcipher,
+ asym_op_info->src, asym_op_info->src_len,
+ asym_op_info->dst, asym_op_info->dst_len, errp);
+ break;
+
+ default:
+ error_setg(errp, "Unknown opcode: %u", op_code);
+ }
+
+ return ret;
+}
+
static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
{
CryptoDevBackendLKCFSession *session = task->sess;
@@ -336,6 +414,7 @@ static void
cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
}
}
+ asym_op_info = task->op_info->u.asym_op_info;
if (key_id < 0) {
if (!qcrypto_akcipher_supports(&session->akcipher_opts)) {
status = -VIRTIO_CRYPTO_NOTSUPP;
@@ -349,72 +428,13 @@ static void
cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
status = -VIRTIO_CRYPTO_ERR;
goto out;
}
- }
-
- asym_op_info = task->op_info->u.asym_op_info;
- switch (op_code) {
- case VIRTIO_CRYPTO_AKCIPHER_ENCRYPT:
- if (key_id >= 0) {
- ret = keyctl_pkey_encrypt(key_id, op_desc,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len);
- } else {
- ret = qcrypto_akcipher_encrypt(akcipher,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len, &local_error);
- }
- break;
- case VIRTIO_CRYPTO_AKCIPHER_DECRYPT:
- if (key_id >= 0) {
- ret = keyctl_pkey_decrypt(key_id, op_desc,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len);
- } else {
- ret = qcrypto_akcipher_decrypt(akcipher,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len, &local_error);
- }
- break;
-
- case VIRTIO_CRYPTO_AKCIPHER_SIGN:
- if (key_id >= 0) {
- ret = keyctl_pkey_sign(key_id, op_desc,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len);
- } else {
- ret = qcrypto_akcipher_sign(akcipher,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len, &local_error);
- }
- break;
-
- case VIRTIO_CRYPTO_AKCIPHER_VERIFY:
- if (key_id >= 0) {
- ret = keyctl_pkey_verify(key_id, op_desc,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len);
- } else {
- ret = qcrypto_akcipher_verify(akcipher,
- asym_op_info->src, asym_op_info->src_len,
- asym_op_info->dst, asym_op_info->dst_len, &local_error);
- }
- break;
-
- default:
- error_setg(&local_error, "Unknown opcode: %u", op_code);
- status = -VIRTIO_CRYPTO_ERR;
- goto out;
+ ret = cryptodev_lkcf_qakcipher_op(akcipher, op_code, op_desc,
asym_op_info, &local_error);
+ } else {
+ ret = cryptodev_lkcf_keyctl_op(key_id, op_code, op_desc,
asym_op_info, &local_error);
}
if (ret < 0) {
- if (!local_error) {
- if (errno != EKEYREJECTED) {
- error_report("Failed do operation with keyctl: %d", errno);
- }
- } else {
- error_report_err(local_error);
- }
status = op_code == VIRTIO_CRYPTO_AKCIPHER_VERIFY ?
-VIRTIO_CRYPTO_KEY_REJECTED : -VIRTIO_CRYPTO_ERR;
} else {
On 3/12/25 20:02, Markus Armbruster wrote:
> zhenwei pi <pizhenwei@bytedance.com> writes:
>
>> On 3/12/25 18:11, Markus Armbruster wrote:
>>> When cryptodev_lkcf_set_op_desc() fails, we report an error, but
>>> continue anyway. This is wrong. We then pass a non-null @local_error
>>> to various functions, which could easily fail error_setv()'s assertion
>>> on failure.
>>>
>>> Fail the function instead.
>>>
>>> When qcrypto_akcipher_new() fails, we fail the function without
>>> reporting the error. This leaks the Error object.
>>>
>>> Add the missing error reporting. This also frees the Error object.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> backends/cryptodev-lkcf.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
>>> index 41cf24b737..352c3e8958 100644
>>> --- a/backends/cryptodev-lkcf.c
>>> +++ b/backends/cryptodev-lkcf.c
>>> @@ -330,6 +330,8 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
>>> cryptodev_lkcf_set_op_desc(&session->akcipher_opts, op_desc,
>>> sizeof(op_desc), &local_error) != 0) {
>>> error_report_err(local_error);
>>> + status = -VIRTIO_CRYPTO_ERR;
>>> + goto out;
>>> } else {
>>> key_id = add_key(KCTL_KEY_TYPE_PKEY, "lkcf-backend-priv-key",
>>> p8info, p8info_len, KCTL_KEY_RING);
>>> @@ -346,6 +348,7 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
>>> session->key, session->keylen,
>>> &local_error);
>>> if (!akcipher) {
>>> + error_report_err(local_error);
>>> status = -VIRTIO_CRYPTO_ERR;
>>> goto out;
>>> }
>>
>> What about moving several 'error_report_err(local_error);' to:
>>
>> out:
>> if (local_error) {
>> error_report_err(local_error);
>> }
>
> I figure you suggest something like the appended patch. But this led me
> to another question. Consider:
>
> asym_op_info = task->op_info->u.asym_op_info;
> switch (op_code) {
> case VIRTIO_CRYPTO_AKCIPHER_ENCRYPT:
> if (key_id >= 0) {
> ret = keyctl_pkey_encrypt(key_id, op_desc,
> asym_op_info->src, asym_op_info->src_len,
> asym_op_info->dst, asym_op_info->dst_len);
>
> When keyctl_pkey_encrypt() fails, @local_error remains unset.
>
> } else {
> ret = qcrypto_akcipher_encrypt(akcipher,
> asym_op_info->src, asym_op_info->src_len,
> asym_op_info->dst, asym_op_info->dst_len, &local_error);
> }
> break;
>
> [More cases that can also fail without setting @local_error]
>
> default:
> error_setg(&local_error, "Unknown opcode: %u", op_code);
> status = -VIRTIO_CRYPTO_ERR;
> goto out;
> }
>
> if (ret < 0) {
>
> The switch failed.
>
> if (!local_error) {
>
> If it failed without setting @local_error, we report a generic error
> *unless* errno is EKEYREJECTED.
>
> Aside: checking errno this far from whatever set it is asking for
> trouble. It gets overwritten easily.
>
> if (errno != EKEYREJECTED) {
> error_report("Failed do operation with keyctl: %d", errno);
> }
>
> If it failed with setting @local_error, we report that error.
>
> } else {
> error_report_err(local_error);
> }
> status = op_code == VIRTIO_CRYPTO_AKCIPHER_VERIFY ?
> -VIRTIO_CRYPTO_KEY_REJECTED : -VIRTIO_CRYPTO_ERR;
>
> Status set to negative value. This will be assigned to task->status
> below.
>
> It can therefore become negative *silently* (i.e. without an error
> report). Why is this okay?
>
> } else {
> status = VIRTIO_CRYPTO_OK;
> asym_op_info->dst_len = ret;
> }
>
> out:
> if (key_id >= 0) {
> keyctl_unlink(key_id, KCTL_KEY_RING);
> }
> task->status = status;
>
>
>
> diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
> index 41cf24b737..0e20797cb3 100644
> --- a/backends/cryptodev-lkcf.c
> +++ b/backends/cryptodev-lkcf.c
> @@ -329,7 +329,8 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
> &local_error) != 0 ||
> cryptodev_lkcf_set_op_desc(&session->akcipher_opts, op_desc,
> sizeof(op_desc), &local_error) != 0) {
> - error_report_err(local_error);
> + status = -VIRTIO_CRYPTO_ERR;
> + goto out;
> } else {
> key_id = add_key(KCTL_KEY_TYPE_PKEY, "lkcf-backend-priv-key",
> p8info, p8info_len, KCTL_KEY_RING);
> @@ -410,10 +411,9 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
> if (ret < 0) {
> if (!local_error) {
> if (errno != EKEYREJECTED) {
> - error_report("Failed do operation with keyctl: %d", errno);
> + error_setg_errno(&local_error,
> + "Failed do operation with keyctl: %d");
> }
> - } else {
> - error_report_err(local_error);
> }
> status = op_code == VIRTIO_CRYPTO_AKCIPHER_VERIFY ?
> -VIRTIO_CRYPTO_KEY_REJECTED : -VIRTIO_CRYPTO_ERR;
> @@ -423,6 +423,9 @@ static void cryptodev_lkcf_execute_task(CryptoDevLKCFTask *task)
> }
>
> out:
> + if (local_error) {
> + error_report_err(local_error);
> + }
> if (key_id >= 0) {
> keyctl_unlink(key_id, KCTL_KEY_RING);
> }
>
zhenwei pi <pizhenwei@bytedance.com> writes: > Hi Markus, > > Current code style seems buggy, I think the main reason is that the Error *errp is not generated at right place. keyctl_pkey_XXX fails without new error, qcrypto_akcipher_XXX fails with new error, but they are in the same switch-case code block. If we can separate crypto operations into two functions - cryptodev_lkcf_keyctl_op and cryptodev_lkcf_qakcipher_op, and the error is generate inside the functions, it may be handled easily. Then applying your changes, it seem more clear. What do you think? Looks like a reasonable cleanup to me. I suggest to proceed as follows. We apply my minimal bug fix as is. You post your cleanup on top. Okay?
On 3/18/25 21:21, Markus Armbruster wrote: > zhenwei pi <pizhenwei@bytedance.com> writes: > >> Hi Markus, >> >> Current code style seems buggy, I think the main reason is that the Error *errp is not generated at right place. keyctl_pkey_XXX fails without new error, qcrypto_akcipher_XXX fails with new error, but they are in the same switch-case code block. If we can separate crypto operations into two functions - cryptodev_lkcf_keyctl_op and cryptodev_lkcf_qakcipher_op, and the error is generate inside the functions, it may be handled easily. Then applying your changes, it seem more clear. What do you think? > > Looks like a reasonable cleanup to me. > > I suggest to proceed as follows. We apply my minimal bug fix as is. > You post your cleanup on top. Okay? > OK!
zhenwei pi <pizhenwei@bytedance.com> writes: > On 3/18/25 21:21, Markus Armbruster wrote: >> zhenwei pi <pizhenwei@bytedance.com> writes: >> >>> Hi Markus, >>> >>> Current code style seems buggy, I think the main reason is that the Error *errp is not generated at right place. keyctl_pkey_XXX fails without new error, qcrypto_akcipher_XXX fails with new error, but they are in the same switch-case code block. If we can separate crypto operations into two functions - cryptodev_lkcf_keyctl_op and cryptodev_lkcf_qakcipher_op, and the error is generate inside the functions, it may be handled easily. Then applying your changes, it seem more clear. What do you think? >> >> Looks like a reasonable cleanup to me. >> >> I suggest to proceed as follows. We apply my minimal bug fix as is. >> You post your cleanup on top. Okay? >> > > OK! Thanks! I'll include the patch in a pull request of error handling fixes. Care to give your Reviewed-by?
© 2016 - 2025 Red Hat, Inc.