Re: [Qemu-devel] [PATCH v5 1/6] qemu-char: Add MSG_CMSG_CLOEXEC flag to recvmsg

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

On 07/23/2012 06:50 PM, Eric Blake wrote:
> On 07/23/2012 07:08 AM, Corey Bryant wrote:
>> Set the close-on-exec flag for the file descriptor received
>> via SCM_RIGHTS.
>>
>
>> +++ b/qemu-char.c
>> @@ -2263,9 +2263,17 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
>>       msg.msg_control = &msg_control;
>>       msg.msg_controllen = sizeof(msg_control);
>>
>> +#ifdef MSG_CMSG_CLOEXEC
>> +    ret = recvmsg(s->fd, &msg, MSG_CMSG_CLOEXEC);
>> +#else
>>       ret = recvmsg(s->fd, &msg, 0);
>> -    if (ret > 0 && s->is_unix)
>> +    if (ret > 0) {
>> +        qemu_set_cloexec(s->fd);
>
> Wrong fd.  You aren't changing cloexec on the socket (s->fd), but on the
> fd that was received via msg (which you don't know at this point in time).
>

Ugh, that's bad.

>> +    }
>> +#endif
>> +    if (ret > 0 && s->is_unix) {
>>           unix_process_msgfd(chr, &msg);
>
> Only here do you know what fd you received.
>
> I would write it more like:
>
> int flags = 0;
> #ifdef MSG_CMSG_CLOEXEC
> flags |= MSG_CMSG_CLOEXEC
> #endif
> ret = recvmsg(s->fd, &msg, flags);
> if (ret > 0 && s->is_unix) {
>      unix_process_msgfd(chr, &msg);
> #ifndef MSG_CMSG_CLOEXEC
>      qemu_set_cloexec(/* fd determined from msg */)
> #endif
> }
>
> which almost implies that unix_process_msgfd() should be the function
> that sets cloexec, but without wasting the time doing so if recvmsg
> already did the job.
>

Thanks for the suggestion and catching this.  I'll take this into 
account in the next version.

-- 
Regards,
Corey