Thanks Sandrine. Revisiting after v2.3 is sounds fine.
On 4/10/20 2:25 AM, Sandrine Bailleux wrote:
On 4/8/20 12:50 AM, Raghu Krishnamurthy wrote:
Thanks Sandrine, Louis,
Agree with both of you. I'm fine with using asserts or panics, as long as we uniformly use it. The change i sent out(review 3845) was because i noticed inconsistency in handling errors. If we do use asserts, all code that checks for mandatory properties, should be changed to assert as opposed to return error code. For optional properties, we can continue to return an error code or print warnings etc.
Yes, I too think consistency is key here. As you said, we need to settle on the expected behaviour and enforce it uniformly across the fconf code. Let's revisit this code after the v2.3 release.
I would like to point out that using asserts here is different from what is documented in the coding guidelines. The guidelines explicitly spells out using asserts for "programming errors". Now, is having a bad DTB considered a programming error? ;) The DTB is platform data as opposed to code. The guidelines might need to be updated based on the conclusion here.
Now that you point it out, and after taking a closer look at , I think I was wrong. A bad DTB seems more like an integration error than a programming error to me, and thus should be handled with runtime checks according to the current guidelines. Incorrect values for mandatory properties should probably be treated as unrecoverable errors (causing a panic) and incorrect/missing optional properties as recoverable errors (issuing a warning message).
Also note the couple of scenarios i mentioned in an earlier email. Platforms like RPI4 don't generally enable TBBR and the DTB image could get corrupt or be modified on purpose. On a release build, this could cause silent corruption. Panic() would avoid this.
In any case, it would be good to settle on the expected behavior for each of these abnormal cases. I don't have a strong preference for asserts or panics here, since each has its pros and cons as both of you called out.