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

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

On 08/13/2012 12:16 PM, Eric Blake wrote:
> On 08/13/2012 07:44 AM, Corey Bryant wrote:
>> I'll send a new version shortly with these updates also.
>>
>
>>>> +
>>>> +        ret = monitor_fdset_dup_fd_add(fdset_id, dupfd);
>>>> +        if (ret == -1) {
>>>> +            close(dupfd);
>>>> +            return -1;
>>>
>>> This function appears to promise a reasonable errno on failure.
>
> Actually, looking at that function again,
>
>
> +int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
> +{
> +    MonFdset *mon_fdset;
> +    MonFdsetFd *mon_fdset_fd_dup;
> +
> +    QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> +        if (mon_fdset->id != fdset_id) {
> +            continue;
> +        }
> +        QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
> +            if (mon_fdset_fd_dup->fd == dup_fd) {
> +                return -1;
> +            }
> +        }
> +        mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup));
> +        mon_fdset_fd_dup->fd = dup_fd;
> +        QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next);
> +        return 0;
> +    }
> +    return -1;
> +}
>
> The only way it could fail is if we are trying to add an fd that is
> already in the set, or if we don't find mon_fdset; both of which would
> indicate logic bugs earlier in our program.  Would it be worth asserting
> that these conditions are impossible, and making this function return
> void (the addition is always successful if it returns, since g_malloc0
> aborts rather than failing with ENOMEM)?

I think what I did in v10 should suffice.  I didn't update 
monitor_fdset_dup_fd_add(), but I did update the calling code.  If the 
call fails then I set errno to EINVAL since (unless there's a bug) the 
only possible error is that the fdset ID was non-existent.

It makes sense to add the asserts, but at this point I'd like to stick 
with what we have in v10 if that's ok.

>
> And the more I think about it, the more I think that qemu_open MUST
> provide a sane errno value on exit, so you need to make sure that all
> exit paths out of qemu_open have a sensible errno (whether or not the
> helper functions also have to leave errno sane is a matter of taste).
>

Yes, I agree.  I went through the code and at this point (with the v10 
patches) we're always setting errno, or calling a library API that 
should be setting it.

-- 
Regards,
Corey