[vlc-devel] [PATCH 09/12] network: io: Remove VLA usages
Rémi Denis-Courmont
remi at remlab.net
Wed Dec 9 11:19:58 CET 2020
Yeah those call sites! They'll busy loop, or cause their respective callers to, if you throw an ENOMEM error.
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...).
Le 9 décembre 2020 11:28:19 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>On 2020-12-09 10:20, Rémi Denis-Courmont wrote:
>> No it is not fine as already explained and as is obvious from reading
>
>> most call sites.
>
>What call sites are you talking about ? I'm talking about these:
>
>*
>https://code.videolan.org/videolan/vlc/-/blob/master/modules/control/cli/cli.c#L515
> int fd = net_Accept(intf, sys->pi_socket_listen);
> if (fd == -1)
> continue;
>
>*
>https://code.videolan.org/videolan/vlc/-/blob/master/modules/control/cli/cli.c#L756
> p_sys->i_socket =
> net_Accept( p_intf, p_sys->pi_socket_listen );
> if( p_sys->i_socket == -1 ) continue;
>
>*
>https://code.videolan.org/videolan/vlc/-/blob/master/modules/stream_out/rtp.c#L1414
> int fd = net_Accept( id->p_stream, id->listen.fd );
> if( fd == -1 )
> continue;
>
>*
>https://code.videolan.org/videolan/vlc/-/blob/master/modules/lua/libs/net.c#L240
> int i_fd = net_Accept( p_this, *ppi_fd );
>
> lua_pushinteger( L, vlclua_fd_map_safe( L, i_fd ) );
>
>There is no other net_Accept() calls.
>
>> -1
>>
>> Le 9 décembre 2020 11:11:20 GMT+02:00, Steve Lhomme
><robux4 at ycbcr.xyz> a
>> écrit :
>>
>> On 2020-12-09 10:06, Rémi Denis-Courmont wrote:
>>
>> Callers don't expect and can't handle ENOMEM error here. If
>you
>> don't
>> understand, abstain from commenting instead of making stupid
>> wrong comments.
>>
>>
>> "Callers don't expect errors here" is very different than
>"Callers don't
>> expect and can't handle ENOMEM error here".
>>
>> And the net_Accept() documentation doesn't mention that. And in
>fact,
>> apart from Lua that just passes on the value, all the callers
>check for
>> -1 and don't care about the actual type of errors.
>>
>> So this fine is perfectly fine.
>>
>> Le 9 décembre 2020 08:50:43 GMT+02:00, Steve Lhomme
>> <robux4 at ycbcr.xyz> a
>> écrit :
>>
>> This is a review of your comment which is a wrong assertion.
>> Keep on topic.
>>
>> On 2020-12-08 16:43, Rémi Denis-Courmont wrote:
>>
>> Please keep your pointless sarcasms out of the code review.
>>
>> Le 8 décembre 2020 17:20:37 GMT+02:00, Steve Lhomme
>> <robux4 at ycbcr.xyz> a
>> écrit :
>>
>> On 2020-12-08 16:12, Rémi Denis-Courmont wrote:
>>
>> Callers don't expect errors here. Polling just has to work.
>>
>> -1
>>
>>
>> yes, there are plenty of return -1, as seen in the patch.
>>
>> Le 8 décembre 2020 16:19:13 GMT+02:00, "Hugo Beauzée-Luyssen"
>> <hugo at beauzee.fr> a écrit :
>>
>------------------------------------------------------------------------
>> src/network/io.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/network/io.c b/src/network/io.c
>> index 5285edc169..d4f15fe115 100644
>> --- a/src/network/io.c
>> +++ b/src/network/io.c
>> @@ -313,7 +313,9 @@ int net_Accept(vlc_object_t *obj, int
>*fds)
>> while (fds[n] != -1)
>> n++;
>>
>> - struct pollfd ufd[n];
>> + struct pollfd *ufd = malloc(n * sizeof((*ufd)));
>> + if (!ufd)
>> + return -1;
>> /* Initialize file descriptor set */
>> for (unsigned i = 0; i < n; i++)
>> {
>> @@ -328,6 +330,7 @@ int net_Accept(vlc_object_t *obj, int
>*fds)
>> if (net_errno != EINTR)
>> {
>> msg_Err(obj, "poll error: %s", vlc_strerror_c(net_errno));
>> + free(ufd);
>> return -1;
>> }
>> }
>> @@ -359,9 +362,11 @@ int net_Accept(vlc_object_t *obj, int
>*fds)
>> */
>> memmove(fds + i, fds + i + 1, n - (i + 1));
>> fds[n - 1] = sfd;
>> + free(ufd);
>> return fd;
>> }
>> }
>> + free(ufd);
>> return -1;
>> }
>>
>>
>>
>> --
>> Envoyé de mon appareil Android avec Courriel K-9 Mail.
>Veuillez
>> excuser
>> ma brièveté.
>>
>------------------------------------------------------------------------
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>
>> <https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>>
>> <https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>
>> <https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>>>
>>
>------------------------------------------------------------------------
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>
>> <https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>>
>> <https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>
>> <https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>>>
>>
>>
>> --
>> Envoyé de mon appareil Android avec Courriel K-9 Mail.
>Veuillez
>> excuser
>> ma brièveté.
>>
>------------------------------------------------------------------------
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>
>> <https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>>
>>
>------------------------------------------------------------------------
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>
>> <https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>>
>>
>>
>> --
>> Envoyé de mon appareil Android avec Courriel K-9 Mail.
>Veuillez
>> excuser
>> ma brièveté.
>>
>------------------------------------------------------------------------
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>> <https://mailman.videolan.org/listinfo/vlc-devel>
>>
>>
>------------------------------------------------------------------------
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
><https://mailman.videolan.org/listinfo/vlc-devel>
>>
>>
>> --
>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>excuser
>> ma brièveté.
>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>>
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel
--
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20201209/3d886c5e/attachment.html>
More information about the vlc-devel
mailing list