cederom commented on PR #17514: URL: https://github.com/apache/nuttx/pull/17514#issuecomment-3677413227
> > @cederom: Shouldn't we have retries += 62 here in place of retries *= 62 ? > @xiaoxiang781216: it is an unrelated topic, please provide a patch to fix instead blocking this patch, thanks. @cederom . looks like this is related, because: 1. someone proposes a fix with really minimal and incomplete description. 2. there may be a different solution to the problem. 3. current change seems quick-fix that changes existing functional behavior. This retry counter that may seem blocking when too big thus firing the wdog (according to description), this blocking may come from using multiplication and not addition in retry calculation what would deplete retries a lot faster, thus my question. did the author even try to replace `*=` with `+=` and see if that fixes the problem? * For some reason **initial code author put the retries in place and did not return on first error** like the change tries to do, so probably retires on errors were expected. * Before change counter was decreased even on errors when `ret<0` and `errno!=ENOENT`, and then at the end there was `set_errno(EINVAL);` to mark error. * After change on first `ret<0` function jumps out on first error without retry and without `set_errno(EINVAL);`. * There is no information nowhere about that functional change - it will jump out on first error (compared to before) and there will be different errno set (compared to before). So there are several issues here, not really mentioned in the descriptions: 1. too long to wait/retry may just come from too big retry counter, thus my question what happens when this counter is decreased (i.e. `+` is used in place of `*`)? 2. function jumps out on first error. before it retried on errors other than expected. yes we may consider it bug but maybe it was there for a reason? 3. different errno after function returns NULL pointer. this is good because we may then know exactly what the problem was. 4. inadequate PR/commit description and testing, also does not explicitly states behavioral change introduced. If PR aligned well right from start to Contributing Guidelines (i.e. provided good description, what changed, why, and how, plus testing logs) it would go smooth. Thus our kindest request please :-) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
