[FFmpeg-devel,v3,1/3] avformat/network: add ff_neterrno2() for cases where we already have an errno

For example, WSAStartup()'s documentation says:

    "A call to the WSAGetLastError function is not needed and should not be used"
---
 libavformat/network.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

On date Friday 2024-04-19 20:07:59 +0100, Andrew Sayers wrote:
> For example, WSAStartup()'s documentation says:
> 
>     "A call to the WSAGetLastError function is not needed and should not be used"
> ---
>  libavformat/network.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

missing declaration in network.h?

On Fri, 19 Apr 2024, Andrew Sayers wrote:

> For example, WSAStartup()'s documentation says:
>
>    "A call to the WSAGetLastError function is not needed and should not be used"
> ---
> libavformat/network.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/network.c b/libavformat/network.c
> index f752efc411..fb70f9cafc 100644
> --- a/libavformat/network.c
> +++ b/libavformat/network.c
> @@ -121,9 +121,14 @@ void ff_network_close(void)
> }
>
> #if HAVE_WINSOCK2_H
> +
> +}
> int ff_neterrno(void)
> {
> -    int err = WSAGetLastError();
> +    return ff_neterrno2(WSAGetLastError());
> +}
> +int ff_neterrno2(int err)

ff_neterror(int err) would be a better name, since it has nothing to do 
with errno.

Regards.
Marton

> +{
>     switch (err) {
>     case WSAEWOULDBLOCK:
>         return AVERROR(EAGAIN);
> -- 
> 2.43.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

On Sun, May 05, 2024 at 09:59:28PM +0200, Marton Balint wrote:
> 
> 
> On Fri, 19 Apr 2024, Andrew Sayers wrote:
> 
> > For example, WSAStartup()'s documentation says:
> > 
> >    "A call to the WSAGetLastError function is not needed and should not be used"
> > ---
> > libavformat/network.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/network.c b/libavformat/network.c
> > index f752efc411..fb70f9cafc 100644
> > --- a/libavformat/network.c
> > +++ b/libavformat/network.c
> > @@ -121,9 +121,14 @@ void ff_network_close(void)
> > }
> > 
> > #if HAVE_WINSOCK2_H
> > +
> > +}
> > int ff_neterrno(void)
> > {
> > -    int err = WSAGetLastError();
> > +    return ff_neterrno2(WSAGetLastError());
> > +}
> > +int ff_neterrno2(int err)
> 
> ff_neterror(int err) would be a better name, since it has nothing to do with
> errno.
> 
> Regards.
> Marton

Not sure which one you're proposing to rename, but if the original
ff_neterrno() was documented, I think it would say something like:

/*
 * @brief AVERROR for the latest network function
 * @return AVERROR based on `WSAGetLastError()` (Windows) or `errno` (otherwise)
 */

... and neterrno2 would be something like:

/*
 * @brief ff_neterrno()-style AVERROR
 * @param err error code (usually an errno or Windows Sockets Error Code)
 * @return AVERROR equivalent to err
 */

So neither necessarily have anything to do with errno.  How about adding the
above documentation then doing s/ff_neterrno(2?)/ff_neterror\1/g ?

Note: this was supposed to be a quick patch documenting the existing behaviour
of a single function, and has now scope-crept up to a fairly sizeable refactor
of a barely-related function.  I don't mind working on this sort of thing in
principle, but would like to land this particular patch before things get too
much further out of hand.

On Tue, 7 May 2024, Andrew Sayers wrote:

> On Sun, May 05, 2024 at 09:59:28PM +0200, Marton Balint wrote:
>>
>>
>> On Fri, 19 Apr 2024, Andrew Sayers wrote:
>>
>>> For example, WSAStartup()'s documentation says:
>>>
>>>    "A call to the WSAGetLastError function is not needed and should not be used"
>>> ---
>>> libavformat/network.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/network.c b/libavformat/network.c
>>> index f752efc411..fb70f9cafc 100644
>>> --- a/libavformat/network.c
>>> +++ b/libavformat/network.c
>>> @@ -121,9 +121,14 @@ void ff_network_close(void)
>>> }
>>>
>>> #if HAVE_WINSOCK2_H
>>> +
>>> +}
>>> int ff_neterrno(void)
>>> {
>>> -    int err = WSAGetLastError();
>>> +    return ff_neterrno2(WSAGetLastError());
>>> +}
>>> +int ff_neterrno2(int err)
>>
>> ff_neterror(int err) would be a better name, since it has nothing to do with
>> errno.
>>
>> Regards.
>> Marton
>
> Not sure which one you're proposing to rename,
> but if the original
> ff_neterrno() was documented, I think it would say something like:
>
> /*
> * @brief AVERROR for the latest network function
> * @return AVERROR based on `WSAGetLastError()` (Windows) or `errno` (otherwise)
> */
>
> ... and neterrno2 would be something like:
>
> /*
> * @brief ff_neterrno()-style AVERROR
> * @param err error code (usually an errno or Windows Sockets Error Code)
> * @return AVERROR equivalent to err
> */
>
> So neither necessarily have anything to do with errno.  How about adding the
> above documentation then doing s/ff_neterrno(2?)/ff_neterror\1/g ?

Okay, indeed that makes the most sense. Since the function renames will 
affect a lot of existing files, it should be a separate patch with no 
functional changes.

Thanks,
Marton

>
> Note: this was supposed to be a quick patch documenting the existing behaviour
> of a single function, and has now scope-crept up to a fairly sizeable refactor
> of a barely-related function.  I don't mind working on this sort of thing in
> principle, but would like to land this particular patch before things get too
> much further out of hand.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

I've prepended two patches - one with just the documentation,
the other with just the code.  The latter was generated automatically
with the command specified in the commit message, so e.g. rebase conflicts
can be resolved by throwing away the old commit and rerunning the command.

As discussed, this also fixes the assigns in if-statements.