kexec-tools: Don't duplicate the bzImage header area

PATCH Superseded Ahmed S. Darwish 2 participants 4 messages Feb 8, 2011 – Feb 9, 2011

Subsystems (2)

Live update kexec handover (KHO)

lore

Share & tools

lore

Ahmed S. Darwish

author

2011-02-08 15:20 UTC #0 lore raw

PATCH 1/1 kexec-tools: Don't duplicate the bzImage header area

Hi,

Don't wholeheartedly copy the 32-Kbytes bzImage header area to the stack, just use a constant pointer instead.

Signed-off-by Ahmed S. Darwish <[email protected]>

---

(If this is accepted, I'll send a similar patch for the setup header)

kexec/arch/i386/kexec-bzImage.c +6 -6

index 83d3a69..2312eb8 100644
@@ -43,32 +43,32 @@ static const int probe_debug = 0;
 
 int bzImage_probe(const char *buf, off_t len)
 {
-	struct x86_linux_header header;
+	const struct x86_linux_header *header;
 	if ((uintmax_t)len < (uintmax_t)sizeof(header)) {
 		return -1;
 	}
-	memcpy(&header, buf, sizeof(header));
-	if (memcmp(header.header_magic, "HdrS", 4) != 0) {
+	header = (void *)buf;
+	if (memcmp(header->header_magic, "HdrS", 4) != 0) {
 		if (probe_debug) {
 			fprintf(stderr, "Not a bzImage\n");
 		}
 		return -1;
 	}
-	if (header.boot_sector_magic != 0xAA55) {
+	if (header->boot_sector_magic != 0xAA55) {
 		if (probe_debug) {
 			fprintf(stderr, "No x86 boot sector present\n");
 		}
 		/* No x86 boot sector present */
 		return -1;
 	}
-	if (header.protocol_version < 0x0200) {
+	if (header->protocol_version < 0x0200) {
 		if (probe_debug) {
 			fprintf(stderr, "Must be at least protocol version 2.00\n");
 		}
 		/* Must be at least protocol version 2.00 */
 		return -1;
 	}
-	if ((header.loadflags & 1) == 0) {
+	if ((header->loadflags & 1) == 0) {
 		if (probe_debug) {
 			fprintf(stderr, "zImage not a bzImage\n");
 		}

thanks,

Signature · 7 lines

Darwish
http://darwish.07.googlepages.com

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Simon Horman

2011-02-08 22:15 UTC #1 lore raw

Quoted 6 lines · Ahmed S. Darwish

Hi,
Don't wholeheartedly copy the 32-Kbytes bzImage header area to the stack, just use a constant pointer instead.
Signed-off-by Ahmed S. Darwish <[email protected]>

Hi Ahmed,

this seems reasonable to me, I have a minor comment below.

Quoted 10 lines

---
(If this is accepted, I'll send a similar patch for the setup header)
kexec/arch/i386/kexec-bzImage.c +0 -0
index 83d3a69..2312eb8 100644
@@ -43,32 +43,32 @@ static const int probe_debug = 0;
 

kexec/arch/i386/kexec-bzImage.c static const int probe_debug = 0; · line 50

Quoted 10 lines

kexec/arch/i386/kexec-bzImage.c +2 -3

 int bzImage_probe(const char *buf, off_t len)
 {
-	struct x86_linux_header header;
+	const struct x86_linux_header *header;
 	if ((uintmax_t)len < (uintmax_t)sizeof(header)) {
 		return -1;
 	}
-	memcpy(&header, buf, sizeof(header));
-	if (memcmp(header.header_magic, "HdrS", 4) != 0) {
+	header = (void *)buf;

Perhaps casting to (struct x86_linux_header*) would read better?

Quoted 28 lines

kexec/arch/i386/kexec-bzImage.c +4 -3

+	if (memcmp(header->header_magic, "HdrS", 4) != 0) {
 		if (probe_debug) {
 			fprintf(stderr, "Not a bzImage\n");
 		}
 		return -1;
 	}
-	if (header.boot_sector_magic != 0xAA55) {
+	if (header->boot_sector_magic != 0xAA55) {
 		if (probe_debug) {
 			fprintf(stderr, "No x86 boot sector present\n");
 		}
 		/* No x86 boot sector present */
 		return -1;
 	}
-	if (header.protocol_version < 0x0200) {
+	if (header->protocol_version < 0x0200) {
 		if (probe_debug) {
 			fprintf(stderr, "Must be at least protocol version 2.00\n");
 		}
 		/* Must be at least protocol version 2.00 */
 		return -1;
 	}
-	if ((header.loadflags & 1) == 0) {
+	if ((header->loadflags & 1) == 0) {
 		if (probe_debug) {
 			fprintf(stderr, "zImage not a bzImage\n");
 		}

Quoted 6 lines

thanks,
Signature · 2 lines
Darwish
http://darwish.07.googlepages.com

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Ahmed S. Darwish

author

2011-02-09 11:13 UTC #2 lore raw

PATCH v2 1/1 kexec-tools: Don't duplicate the bzImage header area

Quoted 8 lines · Simon Horman

Quoted 7 lines · Ahmed S. Darwish
kexec/arch/i386/kexec-bzImage.c +0 -0
index 83d3a69..2312eb8 100644
@@ -43,32 +43,32 @@ static const int probe_debug = 0;
 

Quoted 13 lines

kexec/arch/i386/kexec-bzImage.c static const int probe_debug = 0; · line 50
Quoted 10 lines
kexec/arch/i386/kexec-bzImage.c +2 -3
 int bzImage_probe(const char *buf, off_t len)
 {
-	struct x86_linux_header header;
+	const struct x86_linux_header *header;
 	if ((uintmax_t)len < (uintmax_t)sizeof(header)) {
 		return -1;
 	}
-	memcpy(&header, buf, sizeof(header));
-	if (memcmp(header.header_magic, "HdrS", 4) != 0) {
+	header = (void *)buf;
Perhaps casting to (struct x86_linux_header*) would read better?

I felt that '(const struct x86_linux_header *)' is a bit long, but it's not a big deal. Attached is the same patch with that line modified:

==>

Don't wholeheartedly copy the 32-Kbytes bzImage header area to the stack, just use a constant pointer instead.

Signed-off-by Ahmed S. Darwish <[email protected]>

---

kexec/arch/i386/kexec-bzImage.c +6 -6

index 83d3a69..29e9165 100644
@@ -43,32 +43,32 @@ static const int probe_debug = 0;
 
 int bzImage_probe(const char *buf, off_t len)
 {
-	struct x86_linux_header header;
+	const struct x86_linux_header *header;
 	if ((uintmax_t)len < (uintmax_t)sizeof(header)) {
 		return -1;
 	}
-	memcpy(&header, buf, sizeof(header));
-	if (memcmp(header.header_magic, "HdrS", 4) != 0) {
+	header = (const struct x86_linux_header *)buf;
+	if (memcmp(header->header_magic, "HdrS", 4) != 0) {
 		if (probe_debug) {
 			fprintf(stderr, "Not a bzImage\n");
 		}
 		return -1;
 	}
-	if (header.boot_sector_magic != 0xAA55) {
+	if (header->boot_sector_magic != 0xAA55) {
 		if (probe_debug) {
 			fprintf(stderr, "No x86 boot sector present\n");
 		}
 		/* No x86 boot sector present */
 		return -1;
 	}
-	if (header.protocol_version < 0x0200) {
+	if (header->protocol_version < 0x0200) {
 		if (probe_debug) {
 			fprintf(stderr, "Must be at least protocol version 2.00\n");
 		}
 		/* Must be at least protocol version 2.00 */
 		return -1;
 	}
-	if ((header.loadflags & 1) == 0) {
+	if ((header->loadflags & 1) == 0) {
 		if (probe_debug) {
 			fprintf(stderr, "zImage not a bzImage\n");
 		}

thanks,

Signature · 7 lines

Darwish
http://darwish.07.googlepages.com

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Simon Horman

Applied

2011-02-09 23:25 UTC #3 lore raw

Quoted 23 lines · Ahmed S. Darwish

Quoted 8 lines · Simon Horman
Quoted 7 lines · Ahmed S. Darwish
kexec/arch/i386/kexec-bzImage.c +0 -0
index 83d3a69..2312eb8 100644
@@ -43,32 +43,32 @@ static const int probe_debug = 0;
 
Quoted 13 lines
kexec/arch/i386/kexec-bzImage.c static const int probe_debug = 0;
Quoted 10 lines
kexec/arch/i386/kexec-bzImage.c +2 -3
 int bzImage_probe(const char *buf, off_t len)
 {
-	struct x86_linux_header header;
+	const struct x86_linux_header *header;
 	if ((uintmax_t)len < (uintmax_t)sizeof(header)) {
 		return -1;
 	}
-	memcpy(&header, buf, sizeof(header));
-	if (memcmp(header.header_magic, "HdrS", 4) != 0) {
+	header = (void *)buf;
Perhaps casting to (struct x86_linux_header*) would read better?

Quoted 7 lines

I felt that '(const struct x86_linux_header *)' is a bit long, but it's not a big deal. Attached is the same patch with that line modified:
==>
Don't wholeheartedly copy the 32-Kbytes bzImage header area to the stack, just use a constant pointer instead.

Thanks, applied.

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec