[V6,4/9] virtio-9p: Add qemu side interfaces for chroot environment

QEMU side interfaces to communicate with chroot daemon process.

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 Makefile.objs                   |    2 +-
 hw/9pfs/virtio-9p-chroot-qemu.c |   97 +++++++++++++++++++++++++++++++++++++++
 hw/9pfs/virtio-9p-chroot.h      |    1 +
 3 files changed, 99 insertions(+), 1 deletions(-)
 create mode 100644 hw/9pfs/virtio-9p-chroot-qemu.c

On Mon, Feb 28, 2011 at 11:22 AM, M. Mohan Kumar <mohan@in.ibm.com> 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.

> +    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?

Stefan

On Thursday 03 March 2011 5:08:10 pm Stefan Hajnoczi wrote:
> On Mon, Feb 28, 2011 at 11:22 AM, M. Mohan Kumar <mohan@in.ibm.com> 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.
 
----
M. Mohan Kumar

On Thu, Mar 3, 2011 at 2:01 PM, M. Mohan Kumar <mohan@in.ibm.com> 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 <mohan@in.ibm.com> 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