Sparse image format + Some fixes #5

abozhinov444 · 2019-10-24T08:16:15Z

No description provided.

ndechesne

Some very initial comments.. i haven't reviewed the bulk of the series yet..

please make sure you have signed-off line, and better commit logs. you also need to split patches into individual logical chunks when appropriate.

ndechesne · 2019-10-24T10:22:42Z

firehose.c

@@ -46,11 +46,11 @@
 #include <termios.h>
 #include <time.h>
 #include <unistd.h>
-#include <libxml/parser.h>
-#include <libxml/tree.h>
+


this is not needed (and does not correspond to what you have in the commit log).

libxml/parser.h is unneeded and the libxml/tree.h is included in util.h.
util.h is new header which declares xml parsing wrapper functions, which were previously declared in qdl.h. I don't see by what reason qdl I/O API should be mixed with XML parsing API.

I have updated patch with commit message with all reasons.

ndechesne · 2019-10-24T10:25:01Z

firehose.c

@@ -153,7 +153,8 @@ static int firehose_read(struct qdl_device *qdl, int wait, int (*response_parser
 			for (node = nodes; node; node = node->next) {
 				if (xmlStrcmp(node->name, (xmlChar*)"log") == 0) {
 					firehose_response_log(node);
-				} else if (xmlStrcmp(node->name, (xmlChar*)"response") == 0) {
+				}
+				else if (xmlStrcmp(node->name, (xmlChar*)"response") == 0) {


please do not change code style. or if you do , then you need to do it consistently and in a separate patch anyways.

I will push patch with reverted code style.

I have updated patch with reverted code style.

ndechesne · 2019-10-24T10:27:00Z

patch.c

@@ -102,7 +101,7 @@ int patch_execute(struct qdl_device *qdl, int (*apply)(struct qdl_device *qdl, s
 	int ret;

 	for (patch = patches; patch; patch = patch->next) {
-		if (strcmp(patch->filename, "DISK"))
+		if (xmlStrcmp(patch->filename, (const xmlChar *)"DISK"))


can you justify why/how xmlStrcmp() is better than strcmp()? it would have been a good idea to provide that info in the commit message, btw.

It's better because XML API returns xmlChar which is unsigned char. As we use libxml2 there is no reason to use normal strcmp.

I forgot to mention that the standard does not specify if plain char is signed or unsigned.

I have updated patch with commit message with all reasons.

ndechesne · 2019-10-24T10:28:26Z

program.c

@@ -156,11 +155,11 @@ int program_execute(struct qdl_device *qdl, int (*apply)(struct qdl_device *qdl,
 int program_find_bootable_partition(void)
 {
 	struct program *program;
-	const char *label;
+	char *label;


Why we should use const, when pointer holds dynamically allocated data and dynamically assigned value?

I have updated patch with commit message with all reasons.

ndechesne · 2019-10-24T10:31:06Z

util.h

@@ -0,0 +1,10 @@
+#ifndef __UTIL_H__


this change does not relate to this commit subject

I will update commit message to be more informational.

I have updated patch with commit message with all reasons.

ndechesne · 2019-10-24T10:33:06Z

util.c

@@ -94,7 +94,7 @@ unsigned attr_as_unsigned(xmlNode *node, const char *attr, int *errors)
 	return (unsigned int) strtoul((char*)value, NULL, 10);
 }

-const char *attr_as_string(xmlNode *node, const char *attr, int *errors)
+xmlChar *attr_as_string(xmlNode *node, const char *attr, int *errors)


this function converts a xml attribute into a string. i don't understand why you want to use xmlChar, since the purpose is to extract information from xml.

It's better because XML API returns xmlChar which is unsigned char. As we use libxml2 there is no reason to use other types. With this change things becomes more clear for use.

I have updated patch with commit message with all reasons.

ndechesne · 2019-10-24T10:35:14Z

firehose.c

@@ -103,6 +103,8 @@ static void firehose_response_log(xmlNode *node)

 	value = xmlGetProp(node, (xmlChar*)"value");
 	printf("LOG: %s\n", value);
+


un-needed empty line

Will be removed.

I have updated patch with removed extra line.

ndechesne · 2019-10-24T10:35:51Z

firehose.c


 	value = xmlGetProp(node, (xmlChar*)"value");
-	return !!xmlStrcmp(value, (xmlChar*)"ACK");
+	ret = !!xmlStrcmp(value, (xmlChar*)"ACK");
+


I have updated patch with removed extra line.

ndechesne · 2019-10-24T10:39:26Z

firehose.c

@@ -215,9 +221,14 @@ static int firehose_configure_response_parser(xmlNode *node)
 	size_t max_size;

 	value = xmlGetProp(node, (xmlChar*)"value");
+	if (!value)
+		return -EINVAL;


I am not sure that EINVAL will be interpretated correctly by callers here.

Before my patch it returns also -EINVAL.

right. sorry i missed that ;-)

ndechesne · 2019-10-24T10:45:00Z

sahara.c

@@ -196,8 +196,8 @@ static int sahara_done(struct qdl_device *qdl, struct sahara_pkt *pkt)

 int sahara_run(struct qdl_device *qdl, char *prog_mbn)
 {
-	struct sahara_pkt *pkt;
-	char buf[4096];
+	struct sahara_pkt *pkt = NULL;


these 2 lines are not related to commit log.

I have updated patch with removed extra initialization.

andersson

Some comment inline.

In particular I think it would be nice if you could split this into "freeing of resources", "style changes" and "initialize xml parser".

Thanks,
Bjorn

andersson · 2019-10-29T04:40:12Z

program.c

+		if (programes_last)
+			free(programes_last);
+
+		programes_last = program;


Use a second variable to track the "next" element as you free up the current "program".

for (program = programs; program; program = next) {
next = program->next;
free(program);
}

andersson · 2019-10-29T04:40:47Z

program.c

+
+	for (program = programes; program; program = program->next) {
+		if (program->filename)
+			xmlFree(program->filename);


Analog to standard free() it's valid to pass NULL to this function, so remove the extra conditionals before each one of these.

andersson · 2019-10-29T04:43:37Z

patch.c

+	patches_last = NULL;
+
+	for (patch = patches; patch; patch = patch->next) {
+		if (patch->filename)


Same comments as for program_unload() below (just read your patch backwards).

andersson · 2019-10-29T04:43:55Z

ufs.c

@@ -305,3 +305,22 @@ int ufs_provisioning_execute(struct qdl_device *qdl,
 	}
 	return apply_ufs_epilogue(qdl, ufs_epilogue_p, true);
 }
+
+void ufs_unload(void)


Same as program_unload().

andersson · 2019-10-29T04:45:02Z

qdl.c

@@ -464,6 +477,8 @@ int main(int argc, char **argv)

 	prog_mbn = argv[optind++];

+	xmlInitParser();


This doesn't seem to fall into the same category as the other fixes in this patch; you have freeing of memory, some style changes and this.

I think it would be nice if this was 3 different patches.

andersson

I was under the impression that going towards standard types would be a better choice, but after reading your patch I'm happy with the result. Let's use xmlChar in all places that deals with xml data.

But you have listed three different things you changed in your commit message, that's a good reason to split this into three distinct patches. Please do so.

andersson

LGTM

andersson

I presume you mean that you removed some parts of the API that we don't need.

Otherwise it would be good to track this delta by first adding a verbatim copy of the files, then add a commit that migrates the code to what we want and finally one that adds it to the Makefile.

blenk92 · 2019-10-29T17:10:33Z

I tried this implementation (with valgrind) and I'm seeing this:

==29674== Invalid read of size 8
==29674==    at 0x483F71C: memmove (vg_replace_strmem.c:1271)
==29674==    by 0x10B92C: firehose_program_sparse (firehose.c:330)
==29674==    by 0x10E5FF: callback_stream_skip (output_stream.c:113)
==29674==    by 0x10E38A: write_all_blocks (sparse.c:110)
==29674==    by 0x10E4DC: sparse_stream_callback (sparse.c:141)
==29674==    by 0x10BD2E: firehose_program (firehose.c:444)
==29674==    by 0x10D37E: program_execute (program.c:136)
==29674==    by 0x10C0F2: firehose_run (firehose.c:698)
==29674==    by 0x10AAC8: main (qdl.c:517)
==29674==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==29674== 
==29674== 
==29674== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==29674==  Access not within mapped region at address 0x0
==29674==    at 0x483F71C: memmove (vg_replace_strmem.c:1271)
==29674==    by 0x10B92C: firehose_program_sparse (firehose.c:330)
==29674==    by 0x10E5FF: callback_stream_skip (output_stream.c:113)
==29674==    by 0x10E38A: write_all_blocks (sparse.c:110)
==29674==    by 0x10E4DC: sparse_stream_callback (sparse.c:141)
==29674==    by 0x10BD2E: firehose_program (firehose.c:444)
==29674==    by 0x10D37E: program_execute (program.c:136)
==29674==    by 0x10C0F2: firehose_run (firehose.c:698)
==29674==    by 0x10AAC8: main (qdl.c:517)
==29674==  If you believe this happened as a result of a stack
==29674==  overflow in your program's main thread (unlikely but
==29674==  possible), you can try to increase the size of the
==29674==  main thread stack using the --main-stacksize= flag.
==29674==  The main thread stack size used in this run was 8388608.
==29674== 
...

abozhinov444 · 2019-10-29T18:38:08Z

I tried this implementation (with valgrind) and I'm seeing this:

==29674== Invalid read of size 8
==29674==    at 0x483F71C: memmove (vg_replace_strmem.c:1271)
==29674==    by 0x10B92C: firehose_program_sparse (firehose.c:330)
==29674==    by 0x10E5FF: callback_stream_skip (output_stream.c:113)
==29674==    by 0x10E38A: write_all_blocks (sparse.c:110)
==29674==    by 0x10E4DC: sparse_stream_callback (sparse.c:141)
==29674==    by 0x10BD2E: firehose_program (firehose.c:444)
==29674==    by 0x10D37E: program_execute (program.c:136)
==29674==    by 0x10C0F2: firehose_run (firehose.c:698)
==29674==    by 0x10AAC8: main (qdl.c:517)
==29674==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==29674== 
==29674== 
==29674== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==29674==  Access not within mapped region at address 0x0
==29674==    at 0x483F71C: memmove (vg_replace_strmem.c:1271)
==29674==    by 0x10B92C: firehose_program_sparse (firehose.c:330)
==29674==    by 0x10E5FF: callback_stream_skip (output_stream.c:113)
==29674==    by 0x10E38A: write_all_blocks (sparse.c:110)
==29674==    by 0x10E4DC: sparse_stream_callback (sparse.c:141)
==29674==    by 0x10BD2E: firehose_program (firehose.c:444)
==29674==    by 0x10D37E: program_execute (program.c:136)
==29674==    by 0x10C0F2: firehose_run (firehose.c:698)
==29674==    by 0x10AAC8: main (qdl.c:517)
==29674==  If you believe this happened as a result of a stack
==29674==  overflow in your program's main thread (unlikely but
==29674==  possible), you can try to increase the size of the
==29674==  main thread stack using the --main-stacksize= flag.
==29674==  The main thread stack size used in this run was 8388608.
==29674== 
...

I am splitting patches, now due to latest review and will address this issue.

This change is due to difference between code style on base source and libsparse source.

This change is due to project structure.

- output_file.c to output_stream.c - output_file.h to output_stream.h

- removed unused code - removed crc support - removed gz compression support - renamed all "file" named structures and functions to "stream" in output_stream.c

seperate header, they should not be mixed with qdl I/O API. In result there is a new util.h header.

strcmp to xmlStrcmp. Also removed const qualifiers on dynamically assigned variables.

strcmp to xmlStrcmp and free to xmlFree. Also removed const qualifiers on dynamically assigned variables.

to the library

abozhinov444 · 2019-10-30T07:09:38Z

Hi Bjorn,
i have split all the patches. Also base source is there. Now it is much better. Also i have fixed mentioned above issue.

Regards,
Atanas

blenk92 · 2019-10-30T10:48:09Z

Hi Bjorn,
i have split all the patches. Also base source is there. Now it is much better. Also i have fixed mentioned above issue.

Regards,
Atanas

Tried it, seems to work for me now ;-)

abozhinov444 · 2019-10-30T11:54:36Z

Thank you, for the feedback.

obbardc · 2022-06-16T08:03:07Z

@abozhinov444 Do you have a chance to rebase onto HEAD ?

Based on original work from linux-msm#5 Signed-off-by: Christopher Obbard <[email protected]>

jwinarske · 2023-05-09T21:05:15Z

Is this PR getting merged? What's the status?

ndechesne reviewed Oct 24, 2019

View reviewed changes

abozhinov444 force-pushed the sparse_image_format branch from 190ccba to 68f2a11 Compare October 24, 2019 13:44

andersson requested changes Oct 29, 2019

View reviewed changes

andersson approved these changes Oct 29, 2019

View reviewed changes

andersson reviewed Oct 29, 2019

View reviewed changes

blenk92 mentioned this pull request Oct 29, 2019

Implement sparse image flashing (yet another) #6

Open

abozhinov444 added 21 commits October 29, 2019 21:08

Added libsparse API

f9d386f

libsparse: Ported C++ source to pure C

29e7394

libsparse: Changed code style

9939d51

This change is due to difference between code style on base source and libsparse source.

libsparse: Moved sparse.h to the root

f7abd66

This change is due to project structure.

libsparse: Renamed files

ee5f40f

- output_file.c to output_stream.c - output_file.h to output_stream.h

libsparse: Removed unused executables source

89c6d1a

libsparse: Removed CRC calculation

99eb176

libsparse: Huge rework.

96aef7c

- removed unused code - removed crc support - removed gz compression support - renamed all "file" named structures and functions to "stream" in output_stream.c

program: Added support for sparse property in XML parsing

5ce33ae

firehose: Added support for sparse image format

c3ef5c4

Makefile: Added libsparse source

89f4178

qdl: Splitted XML parsing wrapper functions in new

01caa42

seperate header, they should not be mixed with qdl I/O API. In result there is a new util.h header.

patch: Changed char type variables to xmlChar type and,

aa40d86

strcmp to xmlStrcmp. Also removed const qualifiers on dynamically assigned variables.

program: Changed char type variables to xmlChar type,

91e54b4

strcmp to xmlStrcmp and free to xmlFree. Also removed const qualifiers on dynamically assigned variables.

ufs: Changed char type variables to xmlChar type

6e2d7c0

firehose: Freeing used resources

551c2ea

util: Freeing used resources

8081f22

ufs: New function for freeing resources

f69595f

program: New function for freeing resources

95147c6

patch: New function for freeing resources

e9de7bd

qdl: Init XML parsing stuff before first call

53aae57

to the library

qdl: Freeing used resources

22234e6

abozhinov444 force-pushed the sparse_image_format branch from 68f2a11 to 22234e6 Compare October 30, 2019 07:06

obbardc added a commit to obbardc/qdl that referenced this pull request Jun 29, 2022

Implement sparse image flashing using libsparse

f50d025

Based on original work from linux-msm#5 Signed-off-by: Christopher Obbard <[email protected]>

obbardc mentioned this pull request Jun 29, 2022

Implement sparse image support #30

Draft

		@@ -103,6 +103,8 @@ static void firehose_response_log(xmlNode *node)

		value = xmlGetProp(node, (xmlChar*)"value");
		printf("LOG: %s\n", value);

		@@ -464,6 +477,8 @@ int main(int argc, char **argv)

		prog_mbn = argv[optind++];

		xmlInitParser();

Sparse image format + Some fixes #5

Are you sure you want to change the base?

Sparse image format + Some fixes #5

Conversation

abozhinov444 commented Oct 24, 2019

ndechesne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abozhinov444 Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersson left a comment

Choose a reason for hiding this comment

andersson left a comment

Choose a reason for hiding this comment

andersson left a comment

Choose a reason for hiding this comment

blenk92 commented Oct 29, 2019 • edited Loading

abozhinov444 commented Oct 29, 2019

abozhinov444 commented Oct 30, 2019

blenk92 commented Oct 30, 2019

abozhinov444 commented Oct 30, 2019

obbardc commented Jun 16, 2022

jwinarske commented May 9, 2023

abozhinov444 Oct 24, 2019 •

edited

Loading

blenk92 commented Oct 29, 2019 •

edited

Loading