Re: [libvirt] [PATCH v5 6/6] block: Enable qemu_open/close to work with fd sets

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.

>> + } >> + >> + 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.