[FFmpeg-devel] [libav-devel] [PATCH 2/6] avio: add avio_check()

[Stefano Sabatini]

On date Tuesday 2011-04-12 10:23:54 +0200, Anton Khirnov wrote:
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> 
> The new function is more flexible than url_exist(), as it allows to
> specify which access flags to check, and does not require an explicit
> open of the checked resource.
> 
> Signed-off-by: Anton Khirnov <anton at khirnov.net>
> ---
>  libavformat/avio.c |   19 +++++++++++++++++++
>  libavformat/avio.h |   15 +++++++++++++++
>  libavformat/url.h  |    1 +
>  3 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/libavformat/avio.c b/libavformat/avio.c
> index ad1f1b4..18b2ee6 100644
> --- a/libavformat/avio.c
> +++ b/libavformat/avio.c
> @@ -374,6 +374,25 @@ int url_exist(const char *filename)
>      return 1;
>  }
>  
> +int avio_check(const char *url, int flags)
> +{
> +    URLContext *h;
> +    int ret = ffurl_alloc(&h, url, flags);
> +    if (ret)
> +        return ret;
> +
> +    if (h->prot->url_check) {
> +        ret = h->prot->url_check(h, flags);
> +    } else {
> +        ret = ffurl_connect(h);
> +        if (ret >= 0)
> +            ret = flags;
> +    }
> +
> +    ffurl_close(h);
> +    return ret;
> +}
> +
>  int64_t ffurl_size(URLContext *h)
>  {
>      int64_t pos, size;
> diff --git a/libavformat/avio.h b/libavformat/avio.h
> index b980d49..285c9c7 100644
> --- a/libavformat/avio.h
> +++ b/libavformat/avio.h
> @@ -134,6 +134,7 @@ typedef struct URLProtocol {
>      int priv_data_size;
>      const AVClass *priv_data_class;
>      int flags;
> +    int (*url_check)(URLContext *h, int mask);
>  } URLProtocol;
>  
>  typedef struct URLPollEntry {
> @@ -347,6 +348,20 @@ attribute_deprecated int url_close_buf(AVIOContext *s);
>  int url_exist(const char *url);
>  
>  /**
> + * Return AVIO_* access flags corresponding to the access permissions
> + * of the resource in url, or a negative value corresponding to an
> + * AVERROR code in case of failure. The returned access flags are
> + * masked by the value in flags.
> + *
> + * @note This function is intrinsically unsafe, in the sense that the
> + * checked resource may change its existence or permission status from
> + * one call to another. Thus you should not trust the returned value,
> + * unless you are sure that no other processes are accessing the
> + * checked resource.
> + */
> +int avio_check(const char *url, int flags);

Please don't apply this since there is a problem with it.

#define AVIO_RDONLY 0  /**< read-only */
#define AVIO_WRONLY 1  /**< write-only */
#define AVIO_RDWR   2  /**< read-write */

Indeed AVIO_* are not flags but modes, and it doesn't much sense
OR-ing them, so for example
AVIO_RDONLY|AVIO_WRONLY == AVIO_WRONLY.

and with this patch we have:
avio_check(url, 0) == 0                     -> check for existence
equivalent to
avio_check(url, AVIO_RDONLY) == AVIO_RDONLY -> check for exclusive readability

which doesn't look right (while the fact that we don't open the
resource still avoids the lock, and thus fixes roundup issue 1663).

A possible solution discussed with Anton would be to make AVIO_*
constansts proper flags.

For example we could replace the AVIO_RDONLY and AVIO_WRONLY constants
like this:

#define AVIO_FLAG_RD 1
#define AVIO_FLAG_WR 2

or simply redefine them at the next major bump, but before that the
logic of this avio_check() is broken.

Another possibility:
make avio_check() accept and return flags obtained by shifting 1 by
AVIO_* mode value, for example:

// check for existence
avio_check(url, 0) == 0

// check for exclusive readability
avio_check(url, (1<<AVIO_RDONLY)) > 0

// check for readability
avio_check(url, (1<<AVIO_RDONLY)|(1<<AVIO_RDWR)) > 0

but not very nice from the usability POV.

I'm greatly sorry for not having noticed this problem before.