From mboxrd@z Thu Jan  1 00:00:00 1970
From: Matt Evans <matt@ozlabs.org>
Subject: Re: [PATCH 14/28] kvm tools: Fix term_getc(), term_getc_iov() endian
 bugs
Date: Wed, 07 Dec 2011 13:39:11 +1100
Message-ID: <4EDED1CF.5060004@ozlabs.org>
References: <cover.1323141075.git.matt@ozlabs.org> <4EDD8EBC.5040205@ozlabs.org> <CAOJsxLEJxc==jXq_RtcF_5a_rnwbAKi60CzhrsoL+ujL4t1W5w@mail.gmail.com> <4EDE03F3.4070407@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Cc: Pekka Enberg <penberg@kernel.org>, kvm@vger.kernel.org,
	kvm-ppc@vger.kernel.org, Sasha Levin <levinsasha928@gmail.com>,
	Ingo Molnar <mingo@elte.hu>
To: Asias He <asias.hejun@gmail.com>
Return-path: <kvm-ppc-owner@vger.kernel.org>
In-Reply-To: <4EDE03F3.4070407@gmail.com>
Sender: kvm-ppc-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org

Hi Asias,

On 06/12/11 23:00, Asias He wrote:
> On 12/06/2011 06:24 PM, Pekka Enberg wrote:
>> On Tue, Dec 6, 2011 at 5:40 AM, Matt Evans <matt@ozlabs.org> wrote:
>>> term_getc()'s int c has one byte written into it (at its lowest address) by
>>> read_in_full().  This is expected to be the least significant byte, but that
>>> isn't the case on BE!  Use correct type, unsigned char.  A similar issue exists
>>> in term_getc_iov(), which needs to write a char to the iov rather than an int.
>>>
>>> Signed-off-by: Matt Evans <matt@ozlabs.org>
>>> ---
>>>  tools/kvm/term.c |    5 ++---
>>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/kvm/term.c b/tools/kvm/term.c
>>> index fb5d71c..440884e 100644
>>> --- a/tools/kvm/term.c
>>> +++ b/tools/kvm/term.c
>>> @@ -30,11 +30,10 @@ int term_fds[4][2];
>>>
>>>  int term_getc(int who, int term)
>>>  {
>>> -       int c;
>>> +       unsigned char c;
>>>
>>>        if (who != active_console)
>>>                return -1;
>>> -
> 
> We can drop this line too.
> 
> 	c &= 0xff;

D'oh!  Of course it can -- I'll remove it & repost.

>>>        if (read_in_full(term_fds[term][TERM_FD_IN], &c, 1) < 0)
>>>                return -1;
>>>
>>> @@ -84,7 +83,7 @@ int term_getc_iov(int who, struct iovec *iov, int iovcnt, int term)
>>>        if (c < 0)
>>>                return 0;
>>>
>>> -       *((int *)iov[TERM_FD_IN].iov_base)      = c;
>>> +       *((char *)iov[TERM_FD_IN].iov_base)     = (char)c;
>>>
>>>        return sizeof(char);
>>>  }
>>
>> Looks OK to me. Asias?
>>
> 
> Otherwise, looks fine to me.

Thanks for reviewing!


Matt