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

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

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

Alright, then I'll plan on requiring exact matches of access mode flags.

-- 
Regards,
Corey