The future of ubi_assert()
Richard Weinberger
maintainerArtem,
I'm not happy with ubi_assert().
Currently it only prints a warning and a stack trace but execution continues. In production nobody will notice and while developing turning it into a plain BUG_ON is most of the time more useful because execution stops exactly where the boo boo happens one can analyze stack/registers.
I propose splitting ubi_assert() into two new functions.
1. ubi_bug_on()
Basically a BUG_ON(), it shall be used for assertions where execution of UBI cannot proceed and anything we can do is crashing the machine.
2. ubi_warn_on()
This macro shall be used for assertions where further execution is possible in read-only mode. ubi_warn_on() would be a WARN_ON() plus ubi_ro_mode().
I'm sure that the vast majority of all ubi_asserts() can be turned into a ubi_warn_on().
What do you think?
Thanks,
//richard
Quoted 3 lines · Richard Weinberger
Artem,
I'm not happy with
ubi_assert().
Good start of an e-mail, immediately sets the goal - make Richard happy :-)
Quoted 2 lines
Currently it only prints a warning and a stack trace but execution continues.
Yeah, that was an idea initially, when this all was in process of creation, and we were testing it a lot, and had problems. We did wanted to see a warning, and then let things continue, and see what happens next. And we put really a lot of them, and often they were bogus, and sometimes it was good for production even, because a bogus assert did not stop the whole thing.
Quoted 3 lines
In production nobody will notice and while developing turning
it into a plainBUG_ONis most of the time more useful because execution stops exactly where the boo boo happens one can analyze stack/registers.
Sure, for some of the critical ones it BUG_ON is a better answer.
Quoted 5 lines
I propose splitting
ubi_assert()into two new functions.1.
ubi_bug_on()
Basically aBUG_ON(), it shall be used for assertions where execution of UBI cannot proceed and anything we can do is crashing the machine.
Just use plain BUG_ON() then.
Quoted 3 lines
2.
ubi_warn_on()
This macro shall be used for assertions where further execution is possible in read-only mode.ubi_warn_on()would be aWARN_ON()plusubi_ro_mode().
OK, just use plain WARN_ON(). Many asserts can be just removed even, I think.
Quoted 1 line
I'm sure that the vast majority of all
ubi_asserts()can be turned into aubi_warn_on().
The reason we introduced macros, originally, was that we did not want all our asserts to be compiled in. Because we put them nearly everywhere. Wrapping allowed us to compile them off when debugging was disabled.
But nowadays many of them can be just killed, and we can use WARN_ON() / BUG_ON() without wrapping them.
Richard Weinberger
maintainerAm 06.11.2014 um 08:21 schrieb Artem Bityutskiy:
Quoted 25 lines · Richard Weinberger
Quoted 3 lines
Artem,
I'm not happy with
ubi_assert().Good start of an e-mail, immediately sets the goal - make Richard happy :-)
Quoted 2 lines
Currently it only prints a warning and a stack trace but execution continues.
Yeah, that was an idea initially, when this all was in process of creation, and we were testing it a lot, and had problems. We did wanted to see a warning, and then let things continue, and see what happens next. And we put really a lot of them, and often they were bogus, and sometimes it was good for production even, because a bogus assert did not stop the whole thing.
Quoted 3 lines
In production nobody will notice and while developing turning
it into a plainBUG_ONis most of the time more useful because execution stops exactly where the boo boo happens one can analyze stack/registers.Sure, for some of the critical ones it
BUG_ONis a better answer.
Quoted 7 lines
Quoted 5 lines
I propose splitting
ubi_assert()into two new functions.1.
ubi_bug_on()
Basically aBUG_ON(), it shall be used for assertions where execution of UBI cannot proceed and anything we can do is crashing the machine.Just use plain
BUG_ON()then.
Fine by me.
Quoted 6 lines
Quoted 3 lines
2.
ubi_warn_on()
This macro shall be used for assertions where further execution is possible in read-only mode.ubi_warn_on()would be aWARN_ON()plusubi_ro_mode().OK, just use plain
WARN_ON(). Many asserts can be just removed even, I think.
I think we can do better.
Think of wl.c, if an ubi_assert() in wl.c does not hold, we can put the UBI device into read-only mode and continue execution.BUG_ON() would be too much and WARN_ON() would lead to data corruption as the execution would go on...
Something like ext4's errors=remount-ro. :)
Quoted 6 lines
Quoted 1 line
I'm sure that the vast majority of all
ubi_asserts()can be turned into aubi_warn_on().The reason we introduced macros, originally, was that we did not want all our asserts to be compiled in. Because we put them nearly everywhere. Wrapping allowed us to compile them off when debugging was disabled.
Makes sense.
Quoted 3 lines
But nowadays many of them can be just killed, and we can use
WARN_ON()/BUG_ON()without wrapping them.
Please see my comment above on wl.c.
Thanks,
//richard
Quoted 6 lines · Richard Weinberger
I think we can do better.
Think ofwl.c, if anubi_assert()inwl.cdoes not hold, we can put the UBI device into read-only mode and continue execution.BUG_ON()would be too much andWARN_ON()would lead to data corruption as the execution would go on...
Something like ext4's errors=remount-ro. :)
If you are willing to do the work, and if it is needed, and actually tested, sure.
Richard Weinberger
maintainerAm 06.11.2014 um 09:13 schrieb Artem Bityutskiy:
Quoted 10 lines · Richard Weinberger
Quoted 6 lines
I think we can do better.
Think ofwl.c, if anubi_assert()inwl.cdoes not hold, we can put the UBI device into read-only mode and continue execution.BUG_ON()would be too much andWARN_ON()would lead to data corruption as the execution would go on...
Something like ext4's errors=remount-ro. :)If you are willing to do the work, and if it is needed, and actually tested, sure.
Of course I will.
During fastmap development I ran into basically every ubi_assert() and had to find out whether it is useful or not.^^
Thanks,
//richard