[v2] backends/cryptodev: Do not ignore throttle/backends Errors

[PATCH-for-8.2 v2] backends/cryptodev: Do not ignore throttle/backends Errors

Posted by Philippe Mathieu-Daudé 1 year ago

Both cryptodev_backend_set_throttle() and CryptoDevBackendClass::init()
can set their Error** argument. Do not ignore them, return early on
failure. Use the ERRP_GUARD() macro as suggested in commit ae7c80a7bd
("error: New macro ERRP_GUARD()").

Cc: qemu-stable@nongnu.org
Fixes: e7a775fd9f ("cryptodev: Account statistics")
Fixes: 2580b452ff ("cryptodev: support QoS")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 backends/cryptodev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index e5006bd215..fff89fd62a 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -398,6 +398,7 @@ static void cryptodev_backend_set_ops(Object *obj, Visitor *v,
 static void
 cryptodev_backend_complete(UserCreatable *uc, Error **errp)
 {
+    ERRP_GUARD();
     CryptoDevBackend *backend = CRYPTODEV_BACKEND(uc);
     CryptoDevBackendClass *bc = CRYPTODEV_BACKEND_GET_CLASS(uc);
     uint32_t services;
@@ -406,11 +407,20 @@ cryptodev_backend_complete(UserCreatable *uc, Error **errp)
     QTAILQ_INIT(&backend->opinfos);
     value = backend->tc.buckets[THROTTLE_OPS_TOTAL].avg;
     cryptodev_backend_set_throttle(backend, THROTTLE_OPS_TOTAL, value, errp);
+    if (*errp) {
+        return;
+    }
     value = backend->tc.buckets[THROTTLE_BPS_TOTAL].avg;
     cryptodev_backend_set_throttle(backend, THROTTLE_BPS_TOTAL, value, errp);
+    if (*errp) {
+        return;
+    }
 
     if (bc->init) {
         bc->init(backend, errp);
+        if (*errp) {
+            return;
+        }
     }
 
     services = backend->conf.crypto_services;
-- 
2.41.0

Re: [PATCH-for-8.2 v2] backends/cryptodev: Do not ignore throttle/backends Errors

Posted by Michael Tokarev 10 months, 1 week ago

20.11.2023 18:04, Philippe Mathieu-Daudé:
> Both cryptodev_backend_set_throttle() and CryptoDevBackendClass::init()
> can set their Error** argument. Do not ignore them, return early on
> failure. Use the ERRP_GUARD() macro as suggested in commit ae7c80a7bd
> ("error: New macro ERRP_GUARD()").
> 
> Cc: qemu-stable@nongnu.org
> Fixes: e7a775fd9f ("cryptodev: Account statistics")
> Fixes: 2580b452ff ("cryptodev: support QoS")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Ping?
Can we get this to master before Jan-27? :)
It's been reviewed by a few people already.

Thanks,

/mjt

Re: [PATCH-for-8.2 v2] backends/cryptodev: Do not ignore throttle/backends Errors

Posted by Markus Armbruster 1 year ago

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Both cryptodev_backend_set_throttle() and CryptoDevBackendClass::init()
> can set their Error** argument. Do not ignore them, return early on
> failure.

Let's mention why we need to: "Without that, running into another
failure trips error_setv()'s assertion."

>          Use the ERRP_GUARD() macro as suggested in commit ae7c80a7bd
> ("error: New macro ERRP_GUARD()").
>
> Cc: qemu-stable@nongnu.org
> Fixes: e7a775fd9f ("cryptodev: Account statistics")
> Fixes: 2580b452ff ("cryptodev: support QoS")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  backends/cryptodev.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> index e5006bd215..fff89fd62a 100644
> --- a/backends/cryptodev.c
> +++ b/backends/cryptodev.c
> @@ -398,6 +398,7 @@ static void cryptodev_backend_set_ops(Object *obj, Visitor *v,
>  static void
>  cryptodev_backend_complete(UserCreatable *uc, Error **errp)
>  {
> +    ERRP_GUARD();
>      CryptoDevBackend *backend = CRYPTODEV_BACKEND(uc);
>      CryptoDevBackendClass *bc = CRYPTODEV_BACKEND_GET_CLASS(uc);
>      uint32_t services;
> @@ -406,11 +407,20 @@ cryptodev_backend_complete(UserCreatable *uc, Error **errp)
>      QTAILQ_INIT(&backend->opinfos);
>      value = backend->tc.buckets[THROTTLE_OPS_TOTAL].avg;
>      cryptodev_backend_set_throttle(backend, THROTTLE_OPS_TOTAL, value, errp);
> +    if (*errp) {
> +        return;
> +    }
>      value = backend->tc.buckets[THROTTLE_BPS_TOTAL].avg;
>      cryptodev_backend_set_throttle(backend, THROTTLE_BPS_TOTAL, value, errp);
> +    if (*errp) {
> +        return;
> +    }
>  
>      if (bc->init) {
>          bc->init(backend, errp);
> +        if (*errp) {
> +            return;
> +        }
>      }
>  
>      services = backend->conf.crypto_services;

Preferably with the commit message improved:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

RE: [PATCH-for-8.2 v2] backends/cryptodev: Do not ignore throttle/backends Errors

Posted by Gonglei (Arei) via 1 year ago


> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philmd@linaro.org]
> Sent: Monday, November 20, 2023 11:04 PM
> To: qemu-devel@nongnu.org
> Cc: Zhenwei Pi <pizhenwei@bytedance.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; Markus Armbruster <armbru@redhat.com>;
> Daniel P . Berrangé <berrange@redhat.com>; Philippe Mathieu-Daudé
> <philmd@linaro.org>; qemu-stable@nongnu.org
> Subject: [PATCH-for-8.2 v2] backends/cryptodev: Do not ignore
> throttle/backends Errors
> 
> Both cryptodev_backend_set_throttle() and CryptoDevBackendClass::init() can
> set their Error** argument. Do not ignore them, return early on failure. Use
> the ERRP_GUARD() macro as suggested in commit ae7c80a7bd
> ("error: New macro ERRP_GUARD()").
> 
> Cc: qemu-stable@nongnu.org
> Fixes: e7a775fd9f ("cryptodev: Account statistics")
> Fixes: 2580b452ff ("cryptodev: support QoS")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Gonglei <arei.gonglei@huawei.com>


Regards,
-Gonglei

Re: [PATCH-for-8.2 v2] backends/cryptodev: Do not ignore throttle/backends Errors

Posted by zhenwei pi 1 year ago

Looks good to me. Thanks!

Reviewed-by: zhenwei pi <pizhenwei@bytedance.com>

On 11/20/23 23:04, Philippe Mathieu-Daudé wrote:
> Both cryptodev_backend_set_throttle() and CryptoDevBackendClass::init()
> can set their Error** argument. Do not ignore them, return early on
> failure. Use the ERRP_GUARD() macro as suggested in commit ae7c80a7bd
> ("error: New macro ERRP_GUARD()").
> 
> Cc: qemu-stable@nongnu.org
> Fixes: e7a775fd9f ("cryptodev: Account statistics")
> Fixes: 2580b452ff ("cryptodev: support QoS")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   backends/cryptodev.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> index e5006bd215..fff89fd62a 100644
> --- a/backends/cryptodev.c
> +++ b/backends/cryptodev.c
> @@ -398,6 +398,7 @@ static void cryptodev_backend_set_ops(Object *obj, Visitor *v,
>   static void
>   cryptodev_backend_complete(UserCreatable *uc, Error **errp)
>   {
> +    ERRP_GUARD();
>       CryptoDevBackend *backend = CRYPTODEV_BACKEND(uc);
>       CryptoDevBackendClass *bc = CRYPTODEV_BACKEND_GET_CLASS(uc);
>       uint32_t services;
> @@ -406,11 +407,20 @@ cryptodev_backend_complete(UserCreatable *uc, Error **errp)
>       QTAILQ_INIT(&backend->opinfos);
>       value = backend->tc.buckets[THROTTLE_OPS_TOTAL].avg;
>       cryptodev_backend_set_throttle(backend, THROTTLE_OPS_TOTAL, value, errp);
> +    if (*errp) {
> +        return;
> +    }
>       value = backend->tc.buckets[THROTTLE_BPS_TOTAL].avg;
>       cryptodev_backend_set_throttle(backend, THROTTLE_BPS_TOTAL, value, errp);
> +    if (*errp) {
> +        return;
> +    }
>   
>       if (bc->init) {
>           bc->init(backend, errp);
> +        if (*errp) {
> +            return;
> +        }
>       }
>   
>       services = backend->conf.crypto_services;

-- 
zhenwei pi