* [PATCH] utils/fifo8: change fatal errors from abort() to assert()
@ 2021-01-14 8:33 Mark Cave-Ayland
2021-01-14 9:07 ` Claudio Fontana
0 siblings, 1 reply; 6+ messages in thread
From: Mark Cave-Ayland @ 2021-01-14 8:33 UTC (permalink / raw)
To: qemu-devel, peter.maydell
Developer errors are better represented with assert() rather than abort().
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
This was suggested by Peter during a discussion on IRC yesterday.
---
util/fifo8.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/util/fifo8.c b/util/fifo8.c
index a5dd789ce5..d4d1c135e0 100644
--- a/util/fifo8.c
+++ b/util/fifo8.c
@@ -31,9 +31,7 @@ void fifo8_destroy(Fifo8 *fifo)
void fifo8_push(Fifo8 *fifo, uint8_t data)
{
- if (fifo->num == fifo->capacity) {
- abort();
- }
+ assert(fifo->num < fifo->capacity);
fifo->data[(fifo->head + fifo->num) % fifo->capacity] = data;
fifo->num++;
}
@@ -42,9 +40,7 @@ void fifo8_push_all(Fifo8 *fifo, const uint8_t *data, uint32_t num)
{
uint32_t start, avail;
- if (fifo->num + num > fifo->capacity) {
- abort();
- }
+ assert(fifo->num + num <= fifo->capacity);
start = (fifo->head + fifo->num) % fifo->capacity;
@@ -63,9 +59,7 @@ uint8_t fifo8_pop(Fifo8 *fifo)
{
uint8_t ret;
- if (fifo->num == 0) {
- abort();
- }
+ assert(fifo->num > 0);
ret = fifo->data[fifo->head++];
fifo->head %= fifo->capacity;
fifo->num--;
@@ -76,9 +70,7 @@ const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *num)
{
uint8_t *ret;
- if (max == 0 || max > fifo->num) {
- abort();
- }
+ assert(max > 0 && max <= fifo->num);
*num = MIN(fifo->capacity - fifo->head, max);
ret = &fifo->data[fifo->head];
fifo->head += *num;
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] utils/fifo8: change fatal errors from abort() to assert()
2021-01-14 8:33 [PATCH] utils/fifo8: change fatal errors from abort() to assert() Mark Cave-Ayland
@ 2021-01-14 9:07 ` Claudio Fontana
2021-01-14 9:58 ` Mark Cave-Ayland
0 siblings, 1 reply; 6+ messages in thread
From: Claudio Fontana @ 2021-01-14 9:07 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel, peter.maydell
On 1/14/21 9:33 AM, Mark Cave-Ayland wrote:
> Developer errors are better represented with assert() rather than abort().
... "also, make the tests more strict"
I'd add this since the checks have been changed sometimes in the patch to be more strict.
Reviewed-by: Claudio Fontana <cfontana@suse.de>
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> This was suggested by Peter during a discussion on IRC yesterday.
>
> ---
> util/fifo8.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/util/fifo8.c b/util/fifo8.c
> index a5dd789ce5..d4d1c135e0 100644
> --- a/util/fifo8.c
> +++ b/util/fifo8.c
> @@ -31,9 +31,7 @@ void fifo8_destroy(Fifo8 *fifo)
>
> void fifo8_push(Fifo8 *fifo, uint8_t data)
> {
> - if (fifo->num == fifo->capacity) {
> - abort();
> - }
> + assert(fifo->num < fifo->capacity);
> fifo->data[(fifo->head + fifo->num) % fifo->capacity] = data;
> fifo->num++;
> }
> @@ -42,9 +40,7 @@ void fifo8_push_all(Fifo8 *fifo, const uint8_t *data, uint32_t num)
> {
> uint32_t start, avail;
>
> - if (fifo->num + num > fifo->capacity) {
> - abort();
> - }
> + assert(fifo->num + num <= fifo->capacity);
>
> start = (fifo->head + fifo->num) % fifo->capacity;
>
> @@ -63,9 +59,7 @@ uint8_t fifo8_pop(Fifo8 *fifo)
> {
> uint8_t ret;
>
> - if (fifo->num == 0) {
> - abort();
> - }
> + assert(fifo->num > 0);
> ret = fifo->data[fifo->head++];
> fifo->head %= fifo->capacity;
> fifo->num--;
> @@ -76,9 +70,7 @@ const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *num)
> {
> uint8_t *ret;
>
> - if (max == 0 || max > fifo->num) {
> - abort();
> - }
> + assert(max > 0 && max <= fifo->num);
> *num = MIN(fifo->capacity - fifo->head, max);
> ret = &fifo->data[fifo->head];
> fifo->head += *num;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] utils/fifo8: change fatal errors from abort() to assert()
2021-01-14 9:07 ` Claudio Fontana
@ 2021-01-14 9:58 ` Mark Cave-Ayland
2021-01-14 10:15 ` Claudio Fontana
0 siblings, 1 reply; 6+ messages in thread
From: Mark Cave-Ayland @ 2021-01-14 9:58 UTC (permalink / raw)
To: Claudio Fontana, qemu-devel, peter.maydell
On 14/01/2021 09:07, Claudio Fontana wrote:
> On 1/14/21 9:33 AM, Mark Cave-Ayland wrote:
>> Developer errors are better represented with assert() rather than abort().
>
> ... "also, make the tests more strict"
>
> I'd add this since the checks have been changed sometimes in the patch to be more strict.
>
> Reviewed-by: Claudio Fontana <cfontana@suse.de>
Oh, that was not intentional on my part - I was aiming to keep the same logic but
effectively invert the logic to keep the assert() happy. What did I miss?
ATB,
Mark.
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> This was suggested by Peter during a discussion on IRC yesterday.
>>
>> ---
>> util/fifo8.c | 16 ++++------------
>> 1 file changed, 4 insertions(+), 12 deletions(-)
>>
>> diff --git a/util/fifo8.c b/util/fifo8.c
>> index a5dd789ce5..d4d1c135e0 100644
>> --- a/util/fifo8.c
>> +++ b/util/fifo8.c
>> @@ -31,9 +31,7 @@ void fifo8_destroy(Fifo8 *fifo)
>>
>> void fifo8_push(Fifo8 *fifo, uint8_t data)
>> {
>> - if (fifo->num == fifo->capacity) {
>> - abort();
>> - }
>> + assert(fifo->num < fifo->capacity);
>> fifo->data[(fifo->head + fifo->num) % fifo->capacity] = data;
>> fifo->num++;
>> }
>> @@ -42,9 +40,7 @@ void fifo8_push_all(Fifo8 *fifo, const uint8_t *data, uint32_t num)
>> {
>> uint32_t start, avail;
>>
>> - if (fifo->num + num > fifo->capacity) {
>> - abort();
>> - }
>> + assert(fifo->num + num <= fifo->capacity);
>>
>> start = (fifo->head + fifo->num) % fifo->capacity;
>>
>> @@ -63,9 +59,7 @@ uint8_t fifo8_pop(Fifo8 *fifo)
>> {
>> uint8_t ret;
>>
>> - if (fifo->num == 0) {
>> - abort();
>> - }
>> + assert(fifo->num > 0);
>> ret = fifo->data[fifo->head++];
>> fifo->head %= fifo->capacity;
>> fifo->num--;
>> @@ -76,9 +70,7 @@ const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *num)
>> {
>> uint8_t *ret;
>>
>> - if (max == 0 || max > fifo->num) {
>> - abort();
>> - }
>> + assert(max > 0 && max <= fifo->num);
>> *num = MIN(fifo->capacity - fifo->head, max);
>> ret = &fifo->data[fifo->head];
>> fifo->head += *num;
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] utils/fifo8: change fatal errors from abort() to assert()
2021-01-14 9:58 ` Mark Cave-Ayland
@ 2021-01-14 10:15 ` Claudio Fontana
2021-01-14 11:06 ` Philippe Mathieu-Daudé
2021-01-21 9:50 ` Mark Cave-Ayland
0 siblings, 2 replies; 6+ messages in thread
From: Claudio Fontana @ 2021-01-14 10:15 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel, peter.maydell
On 1/14/21 10:58 AM, Mark Cave-Ayland wrote:
> On 14/01/2021 09:07, Claudio Fontana wrote:
>
>> On 1/14/21 9:33 AM, Mark Cave-Ayland wrote:
>>> Developer errors are better represented with assert() rather than abort().
>>
>> ... "also, make the tests more strict"
>>
>> I'd add this since the checks have been changed sometimes in the patch to be more strict.
>>
>> Reviewed-by: Claudio Fontana <cfontana@suse.de>
>
> Oh, that was not intentional on my part - I was aiming to keep the same logic but
> effectively invert the logic to keep the assert() happy. What did I miss?
Did I misunderstand? Comments below:
>
>
> ATB,
>
> Mark.
>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> This was suggested by Peter during a discussion on IRC yesterday.
>>>
>>> ---
>>> util/fifo8.c | 16 ++++------------
>>> 1 file changed, 4 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/util/fifo8.c b/util/fifo8.c
>>> index a5dd789ce5..d4d1c135e0 100644
>>> --- a/util/fifo8.c
>>> +++ b/util/fifo8.c
>>> @@ -31,9 +31,7 @@ void fifo8_destroy(Fifo8 *fifo)
>>>
>>> void fifo8_push(Fifo8 *fifo, uint8_t data)
>>> {
>>> - if (fifo->num == fifo->capacity) {
>>> - abort();
>>> - }
>>> + assert(fifo->num < fifo->capacity);
This changes the check effectively, the same logic would be in my view:
assert(fifo->num != fifo->capacity);
But I think your change actually makes sense.
>>> fifo->data[(fifo->head + fifo->num) % fifo->capacity] = data;
>>> fifo->num++;
>>> }
>>> @@ -42,9 +40,7 @@ void fifo8_push_all(Fifo8 *fifo, const uint8_t *data, uint32_t num)
>>> {
>>> uint32_t start, avail;
>>>
>>> - if (fifo->num + num > fifo->capacity) {
>>> - abort();
>>> - }
>>> + assert(fifo->num + num <= fifo->capacity);
>>>
>>> start = (fifo->head + fifo->num) % fifo->capacity;
>>>
>>> @@ -63,9 +59,7 @@ uint8_t fifo8_pop(Fifo8 *fifo)
>>> {
>>> uint8_t ret;
>>>
>>> - if (fifo->num == 0) {
>>> - abort();
>>> - }
>>> + assert(fifo->num > 0);
applying the exact same logic would be:
assert(fifo->num != 0);
but again, I think that the actual change is more expressive, and most likely is correct, just more strict.
>>> ret = fifo->data[fifo->head++];
>>> fifo->head %= fifo->capacity;
>>> fifo->num--;
>>> @@ -76,9 +70,7 @@ const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *num)
>>> {
>>> uint8_t *ret;
>>>
>>> - if (max == 0 || max > fifo->num) {
>>> - abort();
>>> - }
>>> + assert(max > 0 && max <= fifo->num);
>>> *num = MIN(fifo->capacity - fifo->head, max);
>>> ret = &fifo->data[fifo->head];
>>> fifo->head += *num;
>>>
>>
>>
>
Ciao,
Claudio
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] utils/fifo8: change fatal errors from abort() to assert()
2021-01-14 10:15 ` Claudio Fontana
@ 2021-01-14 11:06 ` Philippe Mathieu-Daudé
2021-01-21 9:50 ` Mark Cave-Ayland
1 sibling, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-14 11:06 UTC (permalink / raw)
To: Claudio Fontana, Mark Cave-Ayland, qemu-devel, peter.maydell
On 1/14/21 11:15 AM, Claudio Fontana wrote:
> On 1/14/21 10:58 AM, Mark Cave-Ayland wrote:
>> On 14/01/2021 09:07, Claudio Fontana wrote:
>>
>>> On 1/14/21 9:33 AM, Mark Cave-Ayland wrote:
>>>> Developer errors are better represented with assert() rather than abort().
>>>
>>> ... "also, make the tests more strict"
>>>
>>> I'd add this since the checks have been changed sometimes in the patch to be more strict.
>>>
>>> Reviewed-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] utils/fifo8: change fatal errors from abort() to assert()
2021-01-14 10:15 ` Claudio Fontana
2021-01-14 11:06 ` Philippe Mathieu-Daudé
@ 2021-01-21 9:50 ` Mark Cave-Ayland
1 sibling, 0 replies; 6+ messages in thread
From: Mark Cave-Ayland @ 2021-01-21 9:50 UTC (permalink / raw)
To: Claudio Fontana, qemu-devel, Peter Maydell
On 14/01/2021 10:15, Claudio Fontana wrote:
> On 1/14/21 10:58 AM, Mark Cave-Ayland wrote:
>> On 14/01/2021 09:07, Claudio Fontana wrote:
>>
>>> On 1/14/21 9:33 AM, Mark Cave-Ayland wrote:
>>>> Developer errors are better represented with assert() rather than abort().
>>>
>>> ... "also, make the tests more strict"
>>>
>>> I'd add this since the checks have been changed sometimes in the patch to be more strict.
>>>
>>> Reviewed-by: Claudio Fontana <cfontana@suse.de>
>>
>> Oh, that was not intentional on my part - I was aiming to keep the same logic but
>> effectively invert the logic to keep the assert() happy. What did I miss?
>
> Did I misunderstand? Comments below:
>
>>
>>
>> ATB,
>>
>> Mark.
>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> This was suggested by Peter during a discussion on IRC yesterday.
>>>>
>>>> ---
>>>> util/fifo8.c | 16 ++++------------
>>>> 1 file changed, 4 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/util/fifo8.c b/util/fifo8.c
>>>> index a5dd789ce5..d4d1c135e0 100644
>>>> --- a/util/fifo8.c
>>>> +++ b/util/fifo8.c
>>>> @@ -31,9 +31,7 @@ void fifo8_destroy(Fifo8 *fifo)
>>>>
>>>> void fifo8_push(Fifo8 *fifo, uint8_t data)
>>>> {
>>>> - if (fifo->num == fifo->capacity) {
>>>> - abort();
>>>> - }
>>>> + assert(fifo->num < fifo->capacity);
>
> This changes the check effectively, the same logic would be in my view:
>
> assert(fifo->num != fifo->capacity);
>
> But I think your change actually makes sense.
Got it - the difference between using a range check instead of an inequality check :)
>>>> fifo->data[(fifo->head + fifo->num) % fifo->capacity] = data;
>>>> fifo->num++;
>>>> }
>>>> @@ -42,9 +40,7 @@ void fifo8_push_all(Fifo8 *fifo, const uint8_t *data, uint32_t num)
>>>> {
>>>> uint32_t start, avail;
>>>>
>>>> - if (fifo->num + num > fifo->capacity) {
>>>> - abort();
>>>> - }
>>>> + assert(fifo->num + num <= fifo->capacity);
>>>>
>>>> start = (fifo->head + fifo->num) % fifo->capacity;
>>>>
>>>> @@ -63,9 +59,7 @@ uint8_t fifo8_pop(Fifo8 *fifo)
>>>> {
>>>> uint8_t ret;
>>>>
>>>> - if (fifo->num == 0) {
>>>> - abort();
>>>> - }
>>>> + assert(fifo->num > 0);
>
>
> applying the exact same logic would be:
>
> assert(fifo->num != 0);
>
> but again, I think that the actual change is more expressive, and most likely is correct, just more strict.
Agreed. In theory both forms should be the same since these elements are integers,
but I do also prefer being explicit about it being a numeric range.
>>>> ret = fifo->data[fifo->head++];
>>>> fifo->head %= fifo->capacity;
>>>> fifo->num--;
>>>> @@ -76,9 +70,7 @@ const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *num)
>>>> {
>>>> uint8_t *ret;
>>>>
>>>> - if (max == 0 || max > fifo->num) {
>>>> - abort();
>>>> - }
>>>> + assert(max > 0 && max <= fifo->num);
>>>> *num = MIN(fifo->capacity - fifo->head, max);
>>>> ret = &fifo->data[fifo->head];
>>>> fifo->head += *num;
I'll submit a v2 shortly adding your R-B.
ATB,
Mark.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-21 9:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-14 8:33 [PATCH] utils/fifo8: change fatal errors from abort() to assert() Mark Cave-Ayland
2021-01-14 9:07 ` Claudio Fontana
2021-01-14 9:58 ` Mark Cave-Ayland
2021-01-14 10:15 ` Claudio Fontana
2021-01-14 11:06 ` Philippe Mathieu-Daudé
2021-01-21 9:50 ` Mark Cave-Ayland
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.