On Tue, Apr 12, 2011 at 04:00:34PM -0400, Ronald S. Bultje wrote: > 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. >
Yes, pushed with this change. -- Anton Khirnov
signature.asc
Description: Digital signature
_______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
