From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=37705 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1Pkdsy-0005iI-67
	for qemu-devel@nongnu.org; Wed, 02 Feb 2011 09:40:56 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1PkZPl-0004Ou-4O
	for qemu-devel@nongnu.org; Wed, 02 Feb 2011 04:54:18 -0500
Received: from mail-gx0-f173.google.com ([209.85.161.173]:61659)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1PkZPk-0004Oo-Vk
	for qemu-devel@nongnu.org; Wed, 02 Feb 2011 04:54:17 -0500
Received: by gxk28 with SMTP id 28so3247006gxk.4
	for <qemu-devel@nongnu.org>; Wed, 02 Feb 2011 01:54:16 -0800 (PST)
MIME-Version: 1.0
In-Reply-To: <1296537992-16687-1-git-send-email-mohan@in.ibm.com>
References: <1296537693-16406-1-git-send-email-mohan@in.ibm.com>
	<1296537992-16687-1-git-send-email-mohan@in.ibm.com>
Date: Wed, 2 Feb 2011 09:54:16 +0000
Message-ID: <AANLkTimtVxbTjrPdQu7aCst70T61Dwr27S11CyquPH7N@mail.gmail.com>
From: Stefan Hajnoczi <stefanha@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: [Qemu-devel] Re: [V4 PATCH 3/8] Add client side interfaces for
	chroot environment
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: "M. Mohan Kumar" <mohan@in.ibm.com>
Cc: qemu-devel@nongnu.org

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?

*error =3D 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.

Inconsistent use of errno constants and -1:
return -EIO;
return -1; /* =3D=3D -EPERM, probably not what you wanted */

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

> +/*
> + * V9fsFileObjectRequest is written into the socket by QEMU process.
> + * Then this request is read by chroot process using read_request functi=
on
> + */
> +static int v9fs_write_request(int sockfd, V9fsFileObjectRequest *request=
)
> +{
> + =A0 =A0int retval, length;
> + =A0 =A0char *buff, *buffp;
> +
> + =A0 =A0length =3D sizeof(request->data) + request->data.path_len +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0request->data.oldpath_len;
> +
> + =A0 =A0buff =3D qemu_malloc(length);
> + =A0 =A0buffp =3D buff;
> + =A0 =A0memcpy(buffp, &request->data, sizeof(request->data));
> + =A0 =A0buffp +=3D sizeof(request->data);
> + =A0 =A0memcpy(buffp, request->path.path, request->data.path_len);
> + =A0 =A0buffp +=3D request->data.path_len;
> + =A0 =A0memcpy(buffp, request->path.old_path, request->data.oldpath_len)=
;
> +
> + =A0 =A0retval =3D 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.

Stefan