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

[Corey Bryant]

On 07/26/2012 05:07 AM, Kevin Wolf wrote:
> 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.

Yes, we'd continue the loop instead of returning -1.  I prefer to return 
on the first failure, but if anyone feels strongly about continuing the 
loop, please let me know.

--
Regards,
Corey