Hi, On Tue, Apr 12, 2011 at 3:36 PM, Anton Khirnov <[email protected]> wrote: > On Tue, Apr 12, 2011 at 07:34:59PM +0200, Stefano Sabatini wrote: >> On date Tuesday 2011-04-12 10:23:54 +0200, Anton Khirnov wrote: >> > From: Stefano Sabatini <[email protected]> >> > >> > 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 <[email protected]> >> > --- >> > 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. > > I'm in favor of redefining AVIO_* flags during the bump. > > Unles somebody objects, I'll postpone the patch replacing url_exist with > avio_check until the bump and push the rest now, so avio changes can be > finally declared finished. > > During the bump, i'll redefine AVIO_* flags and replace url_exist with > avio_check.
Can you provide a patch that does the AVIO_* flag redefinition now? That way, at least the symbol change is atomic. Thanks, Ronald _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
