ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

PATCH In review Carlo Marcelo Arenas Belon 7 participants 10 messages Jan 4, 2008 – Jan 5, 2008

lore

Share & tools

lore

Carlo Marcelo Arenas Belon

author

2008-01-04 08:01 UTC #0 lore raw

PATCH 1/1 ensure all invocations to bdrv_{read, write} use (uint8_t *) for its third parameter

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

block-vvfat.c +1 -1

diff -u -p -r1.16 block-vvfat.c
@@ -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

block.c +1 -1

diff -u -p -r1.53 block.c
@@ -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;

Thiemo Seufer

2008-01-04 13:20 UTC #1 lore raw

Quoted 2 lines · 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

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

Thiemo

Quoted 36 lines

---
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 +1 -1
@@ -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 +1 -1
@@ -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;

Andreas Färber

2008-01-04 13:41 UTC #2 lore raw

Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:

Quoted 5 lines

Quoted 2 lines · 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
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

Samuel Thibault

2008-01-04 14:01 UTC #3 lore raw

Quoted 4 lines · Andreas Färber, le Fri 04 Jan 2008 14:41:29 +0100, a écrit :

Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
Quoted 1 line
Carlo Marcelo Arenas Belon wrote:

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
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

Andreas Färber

2008-01-04 14:47 UTC #4 lore raw

Am 04.01.2008 um 15:00 schrieb Samuel Thibault:

Quoted 11 lines

Quoted 10 lines · Andreas Färber, le Fri 04 Jan 2008 14:41:29 +0100, a écrit :
Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
Quoted 6 lines
Quoted 3 lines · 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
Do we have a host where this actually makes a difference?

Quoted 7 lines

Quoted 2 lines
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

Thiemo Seufer

2008-01-04 16:14 UTC #5 lore raw

Quoted 21 lines · Andreas Färber

Am 04.01.2008 um 15:00 schrieb Samuel Thibault:
Quoted 18 lines
Quoted 4 lines · Andreas Färber, le Fri 04 Jan 2008 14:41:29 +0100, a écrit :
Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
Quoted 1 line
Carlo Marcelo Arenas Belon wrote:
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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 ...

Quoted 5 lines

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

M. Warner Losh

2008-01-04 17:12 UTC #6 lore raw

In reply to Andreas Färber

In message: <[email protected]>
Andreas_Färber <[email protected]> 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

Andreas Schwab

2008-01-04 18:29 UTC #7 lore raw

In reply to Andreas Färber

Quoted 11 lines · Andreas Färber

Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
Quoted 6 lines
Quoted 3 lines · 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
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.

Signature · 4 lines

Andreas Schwab, SuSE Labs, [email protected]
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."

Rob Landley

2008-01-05 00:39 UTC #8 lore raw

In reply to Andreas Färber

Quoted 10 lines · On Friday 04 January 2008 07:41:29 Andreas Färber wrote:

Am 04.01.2008 um 14:20 schrieb Thiemo Seufer:
Quoted 6 lines
Quoted 3 lines · 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
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).

Quoted 1 line

Andreas

Rob

Signature · 2 lines

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

Carlo Marcelo Arenas Belon

author

2008-01-05 01:51 UTC #9 lore raw

In reply to Thiemo Seufer

Quoted 5 lines · Thiemo Seufer

Quoted 2 lines · 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
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

Quoted 33 lines

Quoted 33 lines
---
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 +1 -1
@@ -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 +1 -1
@@ -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;