[vlc-devel] [PATCH 09/12] network: io: Remove VLA usages
Steve Lhomme
robux4 at ycbcr.xyz
Wed Dec 9 16:05:37 CET 2020
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. The documentation doesn't mention what kind of error they can
expect. 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 as far as I can see poll() can return ENOMEM.
https://www.man7.org/linux/man-pages/man2/poll.2.html
So this case already exist in the current code. Hugo's patch doesn't any
new case to handle on the callers' side.
> 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.
> 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 <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 <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 <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 <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>>>
> <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>>>
> <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>>
> <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
>
More information about the vlc-devel
mailing list