[Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[Carlo Marcelo Arenas Belon]

Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write
are from an array of the uint8_t type

Carlo

---
Index: block-vvfat.c
===================================================================
RCS file: /sources/qemu/qemu/block-vvfat.c,v
retrieving revision 1.16
diff -u -p -r1.16 block-vvfat.c
--- block-vvfat.c	24 Dec 2007 13:26:04 -0000	1.16
+++ block-vvfat.c	4 Jan 2008 07:57:20 -0000
@@ -340,7 +340,7 @@ typedef struct BDRVVVFATState {
     int current_fd;
     mapping_t* current_mapping;
     unsigned char* cluster; /* points to current cluster */
-    unsigned char* cluster_buffer; /* points to a buffer to hold temp data */
+    uint8_t* cluster_buffer; /* points to a buffer to hold temp data */
     unsigned int current_cluster;
 
     /* write support */
Index: block.c
===================================================================
RCS file: /sources/qemu/qemu/block.c,v
retrieving revision 1.53
diff -u -p -r1.53 block.c
--- block.c	24 Dec 2007 16:10:43 -0000	1.53
+++ block.c	4 Jan 2008 07:57:21 -0000
@@ -459,7 +459,7 @@ int bdrv_commit(BlockDriverState *bs)
     BlockDriver *drv = bs->drv;
     int64_t i, total_sectors;
     int n, j;
-    unsigned char sector[512];
+    uint8_t sector[512];
 
     if (!drv)
         return -ENOMEDIUM;

Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[Thiemo Seufer]

Carlo Marcelo Arenas Belon wrote:
> Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write
> are from an array of the uint8_t type

Do we have a host where this actually makes a difference?


Thiemo

> ---
> Index: block-vvfat.c
> ===================================================================
> RCS file: /sources/qemu/qemu/block-vvfat.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 block-vvfat.c
> --- block-vvfat.c	24 Dec 2007 13:26:04 -0000	1.16
> +++ block-vvfat.c	4 Jan 2008 07:57:20 -0000
> @@ -340,7 +340,7 @@ typedef struct BDRVVVFATState {
>      int current_fd;
>      mapping_t* current_mapping;
>      unsigned char* cluster; /* points to current cluster */
> -    unsigned char* cluster_buffer; /* points to a buffer to hold temp data */
> +    uint8_t* cluster_buffer; /* points to a buffer to hold temp data */
>      unsigned int current_cluster;
>  
>      /* write support */
> Index: block.c
> ===================================================================
> RCS file: /sources/qemu/qemu/block.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 block.c
> --- block.c	24 Dec 2007 16:10:43 -0000	1.53
> +++ block.c	4 Jan 2008 07:57:21 -0000
> @@ -459,7 +459,7 @@ int bdrv_commit(BlockDriverState *bs)
>      BlockDriver *drv = bs->drv;
>      int64_t i, total_sectors;
>      int n, j;
> -    unsigned char sector[512];
> +    uint8_t sector[512];
>  
>      if (!drv)
>          return -ENOMEDIUM;
> 
> 
> 

Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[Andreas Färber]

Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:

> Carlo Marcelo Arenas Belon wrote:
>> Trivial fix that ensures that all buffers used for bdrv_read or  
>> bdrv_write
>> are from an array of the uint8_t type
>
> Do we have a host where this actually makes a difference?

I believe Perl makes sizeof(char) checks, so there likely is some  
platform where sizeof(char) > 1.

Andreas

Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[Samuel Thibault]

Andreas Färber, le Fri 04 Jan 2008 14:41:29 +0100, a écrit :
> 
> Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
> 
> >Carlo Marcelo Arenas Belon wrote:
> >>Trivial fix that ensures that all buffers used for bdrv_read or  
> >>bdrv_write
> >>are from an array of the uint8_t type
> >
> >Do we have a host where this actually makes a difference?
> 
> I believe Perl makes sizeof(char) checks, so there likely is some  
> platform where sizeof(char) > 1.

The C standard says

`When applied to an operand that has type char, unsigned char, or signed
char, (or a qualified version thereof) the result is 1.'

Samuel

Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[Andreas Färber]

Am 04.01.2008 um 15:00 schrieb Samuel Thibault:

> Andreas Färber, le Fri 04 Jan 2008 14:41:29 +0100, a écrit :
>>
>> Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
>>
>>> Carlo Marcelo Arenas Belon wrote:
>>>> Trivial fix that ensures that all buffers used for bdrv_read or
>>>> bdrv_write
>>>> are from an array of the uint8_t type
>>>
>>> Do we have a host where this actually makes a difference?
>>
>> I believe Perl makes sizeof(char) checks, so there likely is some
>> platform where sizeof(char) > 1.
>
> The C standard says
>
> `When applied to an operand that has type char, unsigned char, or  
> signed
> char, (or a qualified version thereof) the result is 1.'

The standard maybe. But Win64 violates the C standards, too. ;)

According to our department's ANSI C course, the only consistent rule is
sizeof(char) <= sizeof(short) <= sizeof(int) <= sizeof(long)
without any concrete numbers.

I'm not saying it should be changed or not in QEMU, just saying it may  
not be completely out-of-the-world.

Andreas

Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[Thiemo Seufer]

Andreas Färber wrote:
>
> Am 04.01.2008 um 15:00 schrieb Samuel Thibault:
>
>> Andreas Färber, le Fri 04 Jan 2008 14:41:29 +0100, a écrit :
>>>
>>> Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
>>>
>>>> Carlo Marcelo Arenas Belon wrote:
>>>>> Trivial fix that ensures that all buffers used for bdrv_read or
>>>>> bdrv_write
>>>>> are from an array of the uint8_t type
>>>>
>>>> Do we have a host where this actually makes a difference?
>>>
>>> I believe Perl makes sizeof(char) checks, so there likely is some
>>> platform where sizeof(char) > 1.
>>
>> The C standard says
>>
>> `When applied to an operand that has type char, unsigned char, or signed
>> char, (or a qualified version thereof) the result is 1.'

AFAIR this is C99 ...

> The standard maybe. But Win64 violates the C standards, too. ;)
>
> According to our department's ANSI C course, the only consistent rule is
> sizeof(char) <= sizeof(short) <= sizeof(int) <= sizeof(long)
> without any concrete numbers.

... and this is C89.


Thiemo

Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[M. Warner Losh]

In message: <9C6EAA0B-8DBC-4871-AC1B-6E21B31E92FC@web.de>
            Andreas_Färber <andreas.faerber@web.de> writes:
: 
: Am 04.01.2008 um 15:00 schrieb Samuel Thibault:
: 
: > Andreas Färber, le Fri 04 Jan 2008 14:41:29 +0100, a écrit :
: >>
: >> Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
: >>
: >>> Carlo Marcelo Arenas Belon wrote:
: >>>> Trivial fix that ensures that all buffers used for bdrv_read or
: >>>> bdrv_write
: >>>> are from an array of the uint8_t type
: >>>
: >>> Do we have a host where this actually makes a difference?
: >>
: >> I believe Perl makes sizeof(char) checks, so there likely is some
: >> platform where sizeof(char) > 1.
: >
: > The C standard says
: >
: > `When applied to an operand that has type char, unsigned char, or  
: > signed
: > char, (or a qualified version thereof) the result is 1.'
: 
: The standard maybe. But Win64 violates the C standards, too. ;)
: 
: According to our department's ANSI C course, the only consistent rule is
: sizeof(char) <= sizeof(short) <= sizeof(int) <= sizeof(long)
: without any concrete numbers.
: 
: I'm not saying it should be changed or not in QEMU, just saying it may  
: not be completely out-of-the-world.

The problem is that sizeof(char) has to be 1, or a number of things
will down right fail.  The following code will fail if it doesn't:

     char *a = "abc";
     char *b = malloc(strlen(a) + 1);
     strcpy(b, a);

If sizeof(char) is really 2, then sizeof("abc") is going to be 6, not
3 that strlen returns and the strcpy will smash into memory it doesn't
own.  And *NOBODY*, not even ignorant students, adds a '*
sizeof(char)' to the strlen.  Such a compiler would break just about
every program out there of any significant size.

Now, there's a wchar_t which is the type of the characters in a L
string: "abc"L can be 4, 8, 12, 16 or more bytes in size, but that's
different, and not what's being discussed.

Warner

Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[Andreas Schwab]

Andreas Färber <andreas.faerber@web.de> writes:

> Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
>
>> Carlo Marcelo Arenas Belon wrote:
>>> Trivial fix that ensures that all buffers used for bdrv_read or
>>> bdrv_write
>>> are from an array of the uint8_t type
>>
>> Do we have a host where this actually makes a difference?
>
> I believe Perl makes sizeof(char) checks, so there likely is some platform
> where sizeof(char) > 1.

Not in C.  sizeof(char) == 1 by definition.  Also CHAR_BIT >= 8, so
uint8_t can never be wider than char.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[Rob Landley]

On Friday 04 January 2008 07:41:29 Andreas Färber wrote:
> Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
> > Carlo Marcelo Arenas Belon wrote:
> >> Trivial fix that ensures that all buffers used for bdrv_read or
> >> bdrv_write
> >> are from an array of the uint8_t type
> >
> > Do we have a host where this actually makes a difference?
>
> I believe Perl makes sizeof(char) checks, so there likely is some
> platform where sizeof(char) > 1.

There's a difference between "what some now-obsolete HP minicomputer once did 
in 1987" and "an interesting system with nonzero potential deployments".

A system with sizeof(char)!=1 does not fall in the second category.  In fact, 
on Unix, "short", "int", and "long" all have defined sizes too.

Standard:
  http://www.unix.org/whitepapers/64bit.html
Rationale document:
  http://www.unix.org/version2/whatsnew/lp64_wp.html

Infrastructure in search of a user always bit rots.  Wait for somebody to 
complain, and _then_ add it, once a user has shown up who can test it (and 
detect its absence).

> Andreas

Rob
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.

Re: [Qemu-devel] [PATCH] ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

[Carlo Marcelo Arenas Belon]

On Fri, Jan 04, 2008 at 01:20:39PM +0000, Thiemo Seufer wrote:
> Carlo Marcelo Arenas Belon wrote:
> > Trivial fix that ensures that all buffers used for bdrv_read or bdrv_write
> > are from an array of the uint8_t type
> 
> Do we have a host where this actually makes a difference?

No that I know of (even if I'd heard horror stories about some bizarre old
architecure where sizeof(char) was 3 which I hope I never ever find),
and sorry for stirring this long thread by not being clear about my objective,
and not coming to clarify it before as I had no access until now to my email.

as you said this patch makes no difference in the logic at all in any
architecture that qemu uses (which is why I said it was a trivial fix)
but is IMHO a more correct version of the code because it is consistently
using the same type everywhere instead of different types with different names
in different places, even if they have the same storage requirements; after
all there is a reason why bdrv_read and bdrv_write where defined as using
(uint8_t *) for their buffer parameter since version 1.1 of the vl.h file.

I came to look for it when a merge conflict in kvm flipped the second
invocation from block.c into an "unsigned char *sector[512]" instead as you
can see by the proposed fix here :

  http://article.gmane.org/gmane.comp.emulators.kvm.devel/11510

Carlo

> > ---
> > Index: block-vvfat.c
> > ===================================================================
> > RCS file: /sources/qemu/qemu/block-vvfat.c,v
> > retrieving revision 1.16
> > diff -u -p -r1.16 block-vvfat.c
> > --- block-vvfat.c	24 Dec 2007 13:26:04 -0000	1.16
> > +++ block-vvfat.c	4 Jan 2008 07:57:20 -0000
> > @@ -340,7 +340,7 @@ typedef struct BDRVVVFATState {
> >      int current_fd;
> >      mapping_t* current_mapping;
> >      unsigned char* cluster; /* points to current cluster */
> > -    unsigned char* cluster_buffer; /* points to a buffer to hold temp data */
> > +    uint8_t* cluster_buffer; /* points to a buffer to hold temp data */
> >      unsigned int current_cluster;
> >  
> >      /* write support */
> > Index: block.c
> > ===================================================================
> > RCS file: /sources/qemu/qemu/block.c,v
> > retrieving revision 1.53
> > diff -u -p -r1.53 block.c
> > --- block.c	24 Dec 2007 16:10:43 -0000	1.53
> > +++ block.c	4 Jan 2008 07:57:21 -0000
> > @@ -459,7 +459,7 @@ int bdrv_commit(BlockDriverState *bs)
> >      BlockDriver *drv = bs->drv;
> >      int64_t i, total_sectors;
> >      int n, j;
> > -    unsigned char sector[512];
> > +    uint8_t sector[512];
> >  
> >      if (!drv)
> >          return -ENOMEDIUM;