Syslinux - Aug 2015 - [PATCH 1/2] msg: VGAFilePtr should be char

VGAFilePtr is used as a pointer into VGAFileBuf, so it should be the
same type.

Signed-off-by: Chas Williams <ciwillia at brocade.com>
---
 com32/elflink/ldlinux/msg.c | 4 ++--
 core/graphics.c             | 2 +-
 core/include/graphics.h     | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/com32/elflink/ldlinux/msg.c b/com32/elflink/ldlinux/msg.c
index 9ded33e..37cbe71 100644
--- a/com32/elflink/ldlinux/msg.c
+++ b/com32/elflink/ldlinux/msg.c
@@ -155,7 +155,7 @@ static void msg_filename(uint8_t data)
 
 	/* Ignore space/control char */
 	if (data > ' ') {
-		if ((char *)VGAFilePtr < (VGAFileBuf + sizeof(VGAFileBuf)))
+		if (VGAFilePtr < (VGAFileBuf + sizeof(VGAFileBuf)))
 			*VGAFilePtr++ = data;
 	}
 }
@@ -163,7 +163,7 @@ static void msg_filename(uint8_t data)
 static void msg_vga(void)
 {
 	NextCharJump = msg_filename;
-	VGAFilePtr = (uint16_t *)VGAFileBuf;
+	VGAFilePtr = VGAFileBuf;
 }
 
 static void msg_normal(uint8_t data)
diff --git a/core/graphics.c b/core/graphics.c
index 834372f..1604ab4 100644
--- a/core/graphics.c
+++ b/core/graphics.c
@@ -27,9 +27,9 @@
 
 __export uint8_t UsingVGA = 0;
 uint16_t VGAPos;		/* Pointer into VGA memory */
-__export uint16_t *VGAFilePtr;	/* Pointer into VGAFileBuf */
 __export uint16_t VGAFontSize = 16;	/* Defaults to 16 byte font */
 
+__export char *VGAFilePtr;	                /* Pointer into VGAFileBuf */
 __export char VGAFileBuf[VGA_FILE_BUF_SIZE];	/* Unmangled VGA image name */
 __export char VGAFileMBuf[FILENAME_MAX];	/* Mangled VGA image name */
 
diff --git a/core/include/graphics.h b/core/include/graphics.h
index 814ffe7..65f4adc 100644
--- a/core/include/graphics.h
+++ b/core/include/graphics.h
@@ -41,7 +41,7 @@
 
 extern uint8_t UsingVGA;
 extern uint16_t VGAPos;
-extern uint16_t *VGAFilePtr;
+extern char *VGAFilePtr;
 extern char VGAFileBuf[VGA_FILE_BUF_SIZE];
 extern char VGAFileMBuf[];
 extern uint16_t VGAFontSize;
-- 
2.1.0

>>>>
  VGAFilePtr is used as a pointer into VGAFileBuf, so it should be the same
type.
 
 Signed-off-by: Chas Williams <ciwillia at brocade.com>
 ---
 ....
<<<<


I think probably this patch is buggy:

You have redefined VGAFilePtr

 -__export  uint16_t  *VGAFilePtr;    /* Pointer into VGAFileBuf */  
 +__export char       *VGAFilePtr;    /* Pointer into VGAFileBuf */

but you did not redefine i.e.

 *VGAFilePtr++ = data;

Incrementing a pointer to "uint16_t" implies moving the pointer
forward 2 bytes
while incrementing a pointer to "char" moves the pointer forward only
1 byte.
Then the next pointer de-referencing would point to the wrong location
Was  this code working when you tested it?

Best,
Patrick

On Fri, 2015-08-21 at 12:33 -0700, Patrick Masotta
wrote:
> I think probably this patch is buggy:
> 
> You have redefined VGAFilePtr
> 
>  -__export  uint16_t  *VGAFilePtr;    /* Pointer into VGAFileBuf */  
>  +__export char       *VGAFilePtr;    /* Pointer into VGAFileBuf */
> 
> but you did not redefine i.e.
> 
>  *VGAFilePtr++ = data;
> 
> Incrementing a pointer to "uint16_t" implies moving the pointer
forward 2 bytes
> while incrementing a pointer to "char" moves the pointer forward
only 1 byte.
> Then the next pointer de-referencing would point to the wrong location
I don't understand your point.  Since VGAFilePtr is now a char after
the patch, if you increment it, it only changes by one byte.  That was
the point of the diff in the first place.  Since the types now agree,
you can also get rid of the cast. 

I believe the original author was attempting to use VGAFilterPtr as a
native 16 bit pointer but confused the pointer arithmetic.
> Was  this code working when you tested it?
It works.