[vlc-devel] [PATCH 09/12] network: io: Remove VLA usages

[Steve Lhomme]

On 2020-12-09 16:20, Rémi Denis-Courmont wrote:
> Le mercredi 9 décembre 2020, 17:05:37 EET Steve Lhomme a écrit :
>> On 2020-12-09 11:19, Rémi Denis-Courmont wrote:
>>> Yeah those call sites! They'll busy loop, or cause their respective
>>> callers to, if you throw an ENOMEM error.
>>
>> They will no matter what if there's an "error". It doesn't matter the
>> error.
> 
> Yes it does.
> 
>> The documentation doesn't mention what kind of error they can
>> expect.
> 
> What does have to do with anything? If the documentation disagrees with the
> code, you could argue that one or the other is right. But if there is no
> documentation, then the code ought to be the refence.

In this case the documentation doesn't mention it cannot fail with 
ENOMEM which is in line with the code, which can fail with ENOMEM as 
proved earlier. You're just inventing new rules that exist neither in 
the code nor in the documentation.

>> net_Accept is free to return an error on whatever it wants.
>>
>>    * @return -1 on error (may be transient error due to network issues),
>>    * a new socket descriptor on success.
>>
>> The only case where net_Accept() really returned an error is this:
>>               if (net_errno != EINTR)
>>               {
>>                   msg_Err(obj, "poll error: %s", vlc_strerror_c(net_errno));
>>                   return -1;
>>               }
> 
> And that's what I just said? EBADF/EINVAL can occur if the caller is buggy,
> and it makes sense to log an error to help debug it?

The point here, that you didn't keep in your reply, is ENOMEM which can 
be produced in this code and a return -1 happens. Exactly like Hugo's patch.

>>> I call that a bug. You can't have errors in event handling - other than
>>> events masquerading as errors (EINTR...) and call site bugs (EBADF,
>>> EINVAL...).
>>
>> The fact they are looping eternally on errors is the actual issue. Not
>> that net_Accept may or may have not returned certain types of error only.
> 
> No. We've been through this many times. Event handling cannot fail. If it
> does, depending on the case, it's either a live loop, a dead lock, both at the
> same time (a.k.a. an infinite busy loop).

Are you suggesting net_Accept should not have any return value and 
errors be handled internally ?