| Message ID | 20210114083329.10494-1-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | utils/fifo8: change fatal errors from abort() to assert() | expand |
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; >
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; >> > >
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
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>
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.
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;
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(-)