[Qemu-devel] Re: [V6 PATCH 4/9] virtio-9p: Add qemu side interfaces for chroot environment

[Stefan Hajnoczi]

On Thu, Mar 3, 2011 at 2:01 PM, M. Mohan Kumar <address@hidden> wrote:
> On Thursday 03 March 2011 5:08:10 pm Stefan Hajnoczi wrote:
>> On Mon, Feb 28, 2011 at 11:22 AM, M. Mohan Kumar <address@hidden> wrote:
>> > +    retval = recvmsg(sockfd, &msg, 0);
>> > +    if (retval < 0) {
>> > +        *sock_error = 1;
>> > +        return -EIO;
>> > +    }
>>
>> Are we guaranteed this will be called with signals blocked?  Otherwise
>> we need to handle EINTR.
>
> Ok
>
>>
>> > +    if (fd_info.fi_flags & FI_FD_SOCKERR) {
>> > +        *sock_error = 1;
>> > +        return -EIO;
>> > +    }
>> > +    /* If fd is invalid, ancillary data is not present */
>> > +    if (fd_info.fi_fd < 0 || fd_info.fi_flags & FI_FD_INVALID) {
>> > +        return fd_info.fi_fd;
>> > +    }
>>
>> Testing fd_info.fi_flags & FI_FD_INVALID looks dangerous to me.  If
>> for some reason fi_fd >= 0 then we'd return success here.  fd_fd < 0
>> should be a sufficient check, perhaps you wanted an assert() instead?
>
> This check is required because,
>
> Creating special objects like directory, device nodes will not have a valid
> fd, usually fd will be 0 on success and -ve on error. During success cases, we
> can't do sendmsg for fd=0 value with SCM_RIGHTS, that would result in problem.
> In this case, we set fd_info.fi_flags to FI_FD_INVALID indicating fd is not a
> valid one, but its not an error case also.

+    /* If fd is invalid, ancillary data is not present */
+    if (fd_info.fi_fd < 0 || fd_info.fi_flags & FI_FD_INVALID) {

if (fd_info.fi_fd < 0) {

+        return fd_info.fi_fd;
+    }

We can get here now when no fd has been sent, but that's okay because...

+
+    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
+        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
+                cmsg->cmsg_level != SOL_SOCKET ||
+                cmsg->cmsg_type != SCM_RIGHTS) {
+            continue;
+        }
+        fd = *((int *)CMSG_DATA(cmsg));
+        return fd;
+    }
+    *sock_error = 1;
+    return -EIO;

...no SCM_RIGHTS was sent (it was optional) so instead of considering
this case an error, we have reached a success case without an fd:

return 0;

Stefan