Peter Xu <pet...@redhat.com> writes:
> Sorry Zhijian, I missed this email.
>
> On Wed, Oct 04, 2023 at 06:32:14PM +0200, Juan Quintela wrote:
>> > * Avoid non-negative integer error values.
>
> Perhaps we need to forbid that if doing this.
>
> I can see Zhijian's point, where "if (ret)" can also capture unexpected
> positive returns, while "if (ret < 0)" is not clear on who's handling ret>0
> case. Personally I like that, too.
It's clear either way :)
The problem is calling a function whose contract specifies "return 0 on
success, negative value on failure".
If it returns positive value, the contract is broken, and all bets are
off.
If you check the return value like
if (ret < 0) {
... handle error and fail ...
}
... carry on ...
then an unexpected positive value will clearly be treated as success.
If you check it like
if (ret) {
... handle error and fail ...
}
... carry on ...
then it will clearly be treated as failure.
But we don't know what it is! Treating it as success can be wrong,
treating it as failure can be just as wrong.
>> 3 - I fully agree that code is more maintenable this way. I.e. if any
>> function changes to return positive values for non error, we get
>> better coverage.
>
> This patch does at least try to unify error handling, though..
>
> $ git grep "ret < 0" migration/rdma.c | wc -l
> 36
>
> So we have half doing this and half doing that, makes sense to do the same.
>
> Let's assume no vote from my side due to pros and cons, so it's 2:1. :)
>
> Thanks,
Since I'm not a maintainer here, my vote is advisory.
