On date Wednesday 2011-04-13 08:04:04 +0200, Anton Khirnov wrote: > 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: [...] > > >> 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.
I also suggest to rename the flags to AVIO_FLAG_READ and AVIO_FLAG_WRITE, for consistency and for simplifying the underlying logic (but no need to do it yourself, I'll send a patch as I'll find a bit of time). And of course thanks to Anton for the great work :). _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
