[Qemu-devel] Re: [V4 PATCH 3/8] Add client side interfaces for chroot environment

["M. Mohan Kumar" <mohan@in.ibm.com>]

On Wednesday 02 February 2011 3:24:16 pm Stefan Hajnoczi wrote:
> On Tue, Feb 1, 2011 at 5:26 AM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> > +/* Receive file descriptor and error status from chroot process */
> > +static int v9fs_receivefd(int sockfd, int *error)
> 
> The return value and int *error overlap in functionality.  Would it be
> possible to have only one mechanism for returning errors?
> 

v9fs_receivefd function returns 'fd' on success and returns EIO (and error as 
EIO) if there was a socket error and -1 on other errors.

We need to return -EIO and error as EIO to differentiate between generic EIO 
error and socket failure.
 
> *error = 0 is never done so a caller that passes an uninitialized
> local variable gets back junk when the function succeeds.  It would be
> safer to clear it at the start of this function.
I will update the patch.
> 
> Inconsistent use of errno constants and -1:
> return -EIO;
> return -1; /* == -EPERM, probably not what you wanted */

-1 denotes failure

> 
> How about getting rid of int *error and returning the -errno?  If
> if_error is set then return -fd_info.error.

Do you mean return fd on success and -errno on failure? In this case how to 
differentiate between actual EIO and EIO because of socket read/write failure?

> 
> > +/*
> > + * V9fsFileObjectRequest is written into the socket by QEMU process.
> > + * Then this request is read by chroot process using read_request
> > function + */
> > +static int v9fs_write_request(int sockfd, V9fsFileObjectRequest
> > *request) +{
> > +    int retval, length;
> > +    char *buff, *buffp;
> > +
> > +    length = sizeof(request->data) + request->data.path_len +
> > +                    request->data.oldpath_len;
> > +
> > +    buff = qemu_malloc(length);
> > +    buffp = buff;
> > +    memcpy(buffp, &request->data, sizeof(request->data));
> > +    buffp += sizeof(request->data);
> > +    memcpy(buffp, request->path.path, request->data.path_len);
> > +    buffp += request->data.path_len;
> > +    memcpy(buffp, request->path.old_path, request->data.oldpath_len);
> > +
> > +    retval = qemu_write_full(sockfd, buff, length);
> 
> qemu_free(buff);
> 
> Also, weren't you doing the malloc() + single write() to avoid
> interleaved write()?  Is that still necessary, I thought a mutex was
> introduced?  It's probably worth adding a comment to explain why
> you're doing the malloc + write.

This is used to avoid multiple write system calls. Probably I can update with 
comments. 

----
M. Mohan Kumar