Patchwork [14/28] kvm tools: Fix term_getc(), term_getc_iov() endian bugs

login
register
mail settings
Submitter Matt Evans
Date Dec. 6, 2011, 3:40 a.m.
Message ID <4EDD8EBC.5040205@ozlabs.org>
Download mbox | patch
Permalink /patch/129509/
State New
Headers show

Comments

Matt Evans - Dec. 6, 2011, 3:40 a.m.
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(-)

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pekka Enberg - Dec. 6, 2011, 10:24 a.m.
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;
> -
>        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?
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Asias He - Dec. 6, 2011, noon
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;



>>        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.
Matt Evans - Dec. 7, 2011, 2:39 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;
-
 	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);
 }