Am 26.07.2012 05:57, schrieb Corey Bryant:
On 07/25/2012 03:43 PM, Eric Blake wrote:
On 07/23/2012 07:08 AM, Corey Bryant wrote:
+int monitor_fdset_get_fd(Monitor *mon, int64_t fdset_id, int flags) +{ + mon_fdset_t *mon_fdset; + mon_fdset_fd_t *mon_fdset_fd; + int mon_fd_flags; + + if (!mon) { + errno = ENOENT; + return -1; + } + + QLIST_FOREACH(mon_fdset, &mon->fdsets, next) { + if (mon_fdset->id != fdset_id) { + continue; + } + QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) { + if (mon_fdset_fd->removed) { + continue; + } + + mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL); + if (mon_fd_flags == -1) { + return -1;
This says we fail on the first fcntl() failure, instead of trying other fds in the set. Granted, an fcntl() failure is probably the sign of a bigger bug (such as closing an fd at the wrong point in time), so I guess trying to go on doesn't make much sense once we already know we are hosed.
I think I'll stick with it the way it is. If fcntl() fails we might have a tainted fd set so I think we should fail.
The alternative would be s/return 1/continue/, right? I think either way is acceptable.
+ } + + switch (flags & O_ACCMODE) { + case O_RDWR: + if ((mon_fd_flags & O_ACCMODE) == O_RDWR) { + return mon_fdset_fd->fd; + } + break; + case O_RDONLY: + if ((mon_fd_flags & O_ACCMODE) == O_RDONLY) { + return mon_fdset_fd->fd; + } + break;
Do we want to allow the case where the caller asked for O_RDONLY, but the set only has O_RDWR? After all, the caller is getting a compatible subset of what the set offers.
I don't see a problem with it.
I would require exact matches like you implemented, in order to prevent damage if we ever had a bug that writes to a read-only file. I believe it also makes the semantics clearer and the code simpler, while it shouldn't make much of a difference for clients. Kevin