[PATCH] hab: habv4: Don't reset index when checking for more events

[PATCH] hab: habv4: Don't reset index when checking for more events

[Robin van der Gracht]

If index is reset and the event record pointer is NULL the size of the
indexed event will be written to 'len'.

Currently this results in always printing the buffer overflow error if
there is a HAB event. The index shouldn't be reset to get the size of the
'next' unhandled event. This can occur if there are more events of a single
type than the event buffer (data) can hold.

Telling the user to recompile with an incomplete additional size
suggestion seems useless so I changed it to something more generic.

Signed-off-by: Robin van der Gracht <robin@protonic.nl>
---
 drivers/hab/habv4.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
index e94f82754..3df1d8d3d 100644
--- a/drivers/hab/habv4.c
+++ b/drivers/hab/habv4.c
@@ -504,7 +504,7 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
 {
 	uint8_t data[256];
 	uint32_t len;
-	uint32_t index = 0;
+	uint32_t cnt, index = 0;
 	enum hab_status status;
 	enum hab_config config = 0x0;
 	enum hab_state state = 0x0;
@@ -540,6 +540,7 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
 		len = sizeof(data);
 		index++;
 	}
+	cnt = index;
 
 	len = sizeof(data);
 	index = 0;
@@ -551,12 +552,12 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
 		len = sizeof(data);
 		index++;
 	}
+	cnt += index;
 
-	/* Check reason for stopping */
+	/* Check if there are more events */
 	len = sizeof(data);
-	index = 0;
-	if (rvt->report_event(HAB_STATUS_ANY, index, NULL, &len) == HAB_STATUS_SUCCESS)
-		pr_err("ERROR: Recompile with larger event data buffer (at least %d bytes)\n\n", len);
+	if (rvt->report_event(HAB_STATUS_ANY, cnt, NULL, &len) == HAB_STATUS_SUCCESS)
+		pr_err("WARNING: There are more (undisplayed) events\n");
 
 	return -EPERM;
 }
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] hab: habv4: Don't reset index when checking for more events

[Sascha Hauer]

Hi Robin,

On Fri, Nov 13, 2020 at 03:16:07PM +0100, Robin van der Gracht wrote:
> If index is reset and the event record pointer is NULL the size of the
> indexed event will be written to 'len'.
> 
> Currently this results in always printing the buffer overflow error if
> there is a HAB event. The index shouldn't be reset to get the size of the
> 'next' unhandled event. This can occur if there are more events of a single
> type than the event buffer (data) can hold.
> 
> Telling the user to recompile with an incomplete additional size
> suggestion seems useless so I changed it to something more generic.
> 
> Signed-off-by: Robin van der Gracht <robin@protonic.nl>
> ---
>  drivers/hab/habv4.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
> index e94f82754..3df1d8d3d 100644
> --- a/drivers/hab/habv4.c
> +++ b/drivers/hab/habv4.c
> @@ -504,7 +504,7 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
>  {
>  	uint8_t data[256];
>  	uint32_t len;
> -	uint32_t index = 0;
> +	uint32_t cnt, index = 0;
>  	enum hab_status status;
>  	enum hab_config config = 0x0;
>  	enum hab_state state = 0x0;
> @@ -540,6 +540,7 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
>  		len = sizeof(data);
>  		index++;
>  	}
> +	cnt = index;
>  
>  	len = sizeof(data);
>  	index = 0;
> @@ -551,12 +552,12 @@ static int habv4_get_status(const struct habv4_rvt *rvt)
>  		len = sizeof(data);
>  		index++;
>  	}
> +	cnt += index;
>  
> -	/* Check reason for stopping */
> +	/* Check if there are more events */
>  	len = sizeof(data);
> -	index = 0;
> -	if (rvt->report_event(HAB_STATUS_ANY, index, NULL, &len) == HAB_STATUS_SUCCESS)
> -		pr_err("ERROR: Recompile with larger event data buffer (at least %d bytes)\n\n", len);

You are right, this is wrong. This call will always return
HAB_STATUS_SUCCESS and return the length of the message in len.
So yes, this message is always printed which doesn't make sense.

I think that this whole stuff could be imrpoved. First of all semantics
of the report_event function is meant in a way that first a NULL pointer
is used for the buffer so that the function reports the number of bytes
needed for this event. In the second pass it can be called with a buffer
of exactly this length.
Second I don't think it's good to separate the events parsing into
HAB_STATUS_ERROR and HAB_STATUS_WARNING. If we call report_event with
HAB_STATUS_ANY instead we'll get the events in the order they occured
which probably better helps to make sense of the events.

I've prepared a patch for this, but I currently can't test it, so it
would be great if you could give it a shot. I'll send it out in a
moment.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox