Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
Stephen Warren wrote: > I'll commit to adding a size parameter to the firmware APIs too, within > a day (probably less) of this being in CVS to diff against. Never mind; I have it done already. I'll follow up with the patch and comments/explanations in a few moment.s - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace ___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
Phil Dibowitz wrote:
> Stephen Warren wrote:
>> So, I worked on the patch for issue 1 separately, because I started it
>> first, and because it initially looked like we weren't going to address
>> issue 2. My plan was to fix (1), then quickly work up the fix to (2)
>> following that, to keep the amount of merge issues lower during
>> development.
>
> I see. Well I'd prefer to merge one that does both, but would accept one
> that does just 1 if one to handle 2 is definitely coming shortly there after.
So, here's the patch updated to fold verify_xml_config into
find_config_binary.
I'll commit to adding a size parameter to the firmware APIs too, within
a day (probably less) of this being in CVS to diff against.
Index: concordance/concordance.c
===
RCS file: /cvsroot/concordance/concordance/concordance/concordance.c,v
retrieving revision 1.12
diff -u -p -r1.12 concordance.c
--- concordance/concordance.c 28 Mar 2008 07:50:50 - 1.12
+++ concordance/concordance.c 29 Mar 2008 03:21:54 -
@@ -158,7 +158,7 @@ int dump_config(struct options_t *option
return err;
}
- if ((err = write_config_to_file(config, file_name, size,
+ if ((err = write_config_to_file(config, size, file_name,
(*options).binary))) {
return err;
}
@@ -197,31 +197,20 @@ int upload_config(char *file_name, struc
uint8_t *data;
uint32_t size = 0;
- uint8_t *place_ptr;
- uint32_t binsize;
+ uint8_t *binary_data;
+ uint32_t binary_size;
read_config_from_file(file_name, &data, &size);
- place_ptr = data;
- binsize = size;
+ binary_data = data;
+ binary_size = size;
if (!(*options).binary) {
-
- if ((err = verify_xml_config(data, size)))
- return LC_ERROR;
-
- if ((err = find_config_binary_size(data, &binsize)))
- return LC_ERROR;
-
- /* We no longer need size, let it get munged... */
- if ((err = find_config_binary_start(&place_ptr, &size)))
- return LC_ERROR;
-
- if (size < binsize)
+ if ((err = find_config_binary(data, size, &binary_data, &binary_size)))
return LC_ERROR;
if (!(*options).noweb)
- post_preconfig(data);
+ post_preconfig(data, size);
}
/*
@@ -239,14 +228,14 @@ int upload_config(char *file_name, struc
* erase the flash (to 1) in order to write the flash.
*/
printf("Erasing Flash: ");
- if ((err = erase_config(binsize, cb, (void *)0))) {
+ if ((err = erase_config(binary_size, cb, (void *)0))) {
delete_blob(data);
return err;
}
printf(" done\n");
printf("Writing Config: ");
- if ((err = write_config_to_remote(place_ptr, binsize, cb,
+ if ((err = write_config_to_remote(binary_data, binary_size, cb,
(void *)1))) {
delete_blob(data);
return err;
@@ -254,7 +243,7 @@ int upload_config(char *file_name, struc
printf(" done\n");
printf("Verifying Config:");
- if ((err = verify_remote_config(place_ptr, binsize, cb, (void *)1))) {
+ if ((err = verify_remote_config(binary_data, binary_size, cb, (void *)1))) {
delete_blob(data);
return err;
}
@@ -262,7 +251,7 @@ int upload_config(char *file_name, struc
if (!(*options).binary && !(*options).noweb) {
printf("Contacting website: ");
- if ((err = post_postconfig(data))) {
+ if ((err = post_postconfig(data, size))) {
delete_blob(data);
return err;
}
@@ -298,6 +287,7 @@ int upload_firmware(char *file_name, str
{
int err;
uint8_t *firmware;
+ uint32_t firmware_size;
uint8_t *firmware_bin;
err = 0;
@@ -323,38 +313,48 @@ int upload_firmware(char *file_name, str
}
if ((err = read_firmware_from_file(file_name, &firmware,
- (*options).binary))) {
+ &firmware_size, (*options).binary))) {
delete_blob(firmware);
return err;
}
- if ((err = extract_firmware_binary(firmware, &firmware_bin))) {
- delete_blob(firmware);
- delete_blob(firmware_bin);
- return err;
+ if ((*options).binary) {
+ firmware_bin = firmware;
+ } else {
+ if ((err = extract_firmware_binary(firmware, firmware_size, &firmware_bin))) {
+ delete_blob(firmware_bin);
+ delete_blob(firmware);
+ return err;
+ }
}
if (!(*options).direct) {
if ((err = prep_firmware())) {
printf("Failed to prepare remote for FW update\n");
+ if (firmware_bin != firmware) {
+delete_blob(firmware_bin);
+ }
delete_blob(firmware);
- delete_blob(firmware_bin);
return err;
}
}
printf("Invalidating Flash: ");
if ((err = invalidate_flash())) {
+ if (firmware_bin != firmware) {
+ delete_blob(firmware_bin);
+ }
delete_blob(firmware);
- delete_blob(firmware_bin);
return err;
}
printf(" done\n");
printf("Erasing Flash: ");
if ((err = erase_firmware((*options).direct, cb, (void *)0))) {
+ if (firmware_bin != firmware) {
+ delete_blob(firmware_bin);
+ }
delete_blob(firmware);
- delete_blob(firmware_bin);
return err;
}
printf(" done\n");
@@ -362,13 +362,18 @@ int upload_firmware(char *file_name, str
printf("Writing
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
Stephen Warren wrote: > So, I worked on the patch for issue 1 separately, because I started it > first, and because it initially looked like we weren't going to address > issue 2. My plan was to fix (1), then quickly work up the fix to (2) > following that, to keep the amount of merge issues lower during > development. I see. Well I'd prefer to merge one that does both, but would accept one that does just 1 if one to handle 2 is definitely coming shortly there after. > Yes. I was tempted to simply remove one function or the other, and make a > single uber-function that performed both the complete validation, and > located the binary firmware. It's true such a function would seemingly do > quite a lot, but it's probably easiest for an application to use. Are you > OK with that? I think it probably makes the most sense. That one function, perhaps might end up as a few sub-functions, if necessary for code readability and factorization, but I think having one API call to do both probably makes sense. To be clear, here's *why* I think it makes sense: the verification is *purely* verifying what the XML says *about* the binary blob. So really it's just a safety check we do as part of extracting the binary. If it was verifying other parts of the XML, I would disagree and say that the user should be able to "want to verify the data without looking for the config binary", but given the verification that we're doing, that doesn't actually make sense and it was probably a mistake for me to separate them in the first place. -- Phil Dibowitz [EMAIL PROTECTED] Open Source software and tech docsInsanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ "Never write it in C if you can do it in 'awk'; Never do it in 'awk' if 'sed' can handle it; Never use 'sed' when 'tr' can do the job; Never invoke 'tr' when 'cat' is sufficient; Avoid using 'cat' whenever possible" -- Taylor's Laws of Programming signature.asc Description: OpenPGP digital signature - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
On Thu, March 27, 2008 11:03 pm, Phil Dibowitz wrote: > Stephen Warren wrote: >> On Wed, March 26, 2008 11:55 pm, Phil Dibowitz wrote: >>> Stephen Warren wrote: That was easy - I'd just accidentally included the hunk that fixed the C++ v.s. C comment at the end of libharmony.h, which you already committed separately, which caused the conflict. So, here's the updated version, attached. >>> OK, some more comments. >>> >>> extract_firmware_binary() >>> >>> I thought the whole _point_ of this patch was to do this dynamically, >>> but yet you're still assuming FIRMWARE_SIZE >> >> This change was just to fix all the APIs for data other than the >> firmware blob itself. A second pass could be made to fix up all the >> firmware related APIs. > > That can't be, because before my recent patch (i.e. when you first > purposed this patch) the firmware API *only* returned a blob. And it > returned a blob of FIRMWARE_SIZE and that's _specifically_ what you > objected to. I objected to two separate length-related issues: 1) All the XML parsing use NULL-terminated blobs instead of an explicit length of the blob. 2) The firmware-related APIs didn't have a length parameter. >From what I recall, you seemed OK fixing the first of these issues, pending review of the code for such a fix. As such, I worked up the patch I provided. For the second issue, you initially weren't sure the API should be changed, but then later agreed. So, I worked on the patch for issue 1 separately, because I started it first, and because it initially looked like we weren't going to address issue 2. My plan was to fix (1), then quickly work up the fix to (2) following that, to keep the amount of merge issues lower during development. >>> verify_xml_config() >>> >>> You removed the size-check - why? >> >> It got moved into find_config_binary, since it's a common check for both >> functions. > > Hmm. I see. > > But we expect the user to call verify_xml_config() and then > find_config_binary()... yet, in your code, verify_xml_config() calls > find_config_binary _as_ _well_! This seems silly. Yes. I was tempted to simply remove one function or the other, and make a single uber-function that performed both the complete validation, and located the binary firmware. It's true such a function would seemingly do quite a lot, but it's probably easiest for an application to use. Are you OK with that? - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace ___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
Stephen Warren wrote: > On Wed, March 26, 2008 11:55 pm, Phil Dibowitz wrote: >> Stephen Warren wrote: >>> That was easy - I'd just accidentally included the hunk that fixed the >>> C++ v.s. C comment at the end of libharmony.h, which you already >>> committed separately, which caused the conflict. >>> >>> So, here's the updated version, attached. >> OK, some more comments. >> >> extract_firmware_binary() >> >> I thought the whole _point_ of this patch was to do this dynamically, but >> yet you're still assuming FIRMWARE_SIZE > > This change was just to fix all the APIs for data other than the firmware > blob itself. A second pass could be made to fix up all the firmware related > APIs. That can't be, because before my recent patch (i.e. when you first purposed this patch) the firmware API *only* returned a blob. And it returned a blob of FIRMWARE_SIZE and that's _specifically_ what you objected to. Of course, after my recent change, I started reading in the XML as well, without passing back a size, but your objection came before that patch when firmware only ever dealt with a blob. If we're going to change the API "for consistency" then let's be consistent. >> verify_xml_config() >> >> You removed the size-check - why? > > It got moved into find_config_binary, since it's a common check for both > functions. Hmm. I see. But we expect the user to call verify_xml_config() and then find_config_binary()... yet, in your code, verify_xml_config() calls find_config_binary _as_ _well_! This seems silly. Perhaps we should expect the user to call find_config_binary() first and then pass in the relevant pointers to verify_xml_config()? Or perhaps we should combine the functions such that there's just find_config_binary(), and it automatically does the verification and returns an error if there's a problem (or, I suppose, we could pass in a flag on whether or not we want the verification done)? I think I like the later better... but I'm willing to listen to any arguments. -- Phil Dibowitz [EMAIL PROTECTED] Open Source software and tech docsInsanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ "Never write it in C if you can do it in 'awk'; Never do it in 'awk' if 'sed' can handle it; Never use 'sed' when 'tr' can do the job; Never invoke 'tr' when 'cat' is sufficient; Avoid using 'cat' whenever possible" -- Taylor's Laws of Programming signature.asc Description: OpenPGP digital signature - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
On Wed, March 26, 2008 11:55 pm, Phil Dibowitz wrote:
> As I mentioned, I'd like the for(;;)s to be while(1)s.
Here you go; attached.
Index: concordance/concordance.c
===
RCS file: /cvsroot/concordance/concordance/concordance/concordance.c,v
retrieving revision 1.11
diff -u -p -r1.11 concordance.c
--- concordance/concordance.c 26 Mar 2008 01:33:31 - 1.11
+++ concordance/concordance.c 28 Mar 2008 03:08:36 -
@@ -157,7 +157,7 @@ int dump_config(struct options_t *option
return err;
}
- if ((err = write_config_to_file(config, file_name, size,
+ if ((err = write_config_to_file(config, size, file_name,
(*options).binary))) {
return err;
}
@@ -196,31 +196,23 @@ int upload_config(char *file_name, struc
uint8_t *data;
uint32_t size = 0;
- uint8_t *place_ptr;
- uint32_t binsize;
+ uint8_t *binary_data;
+ uint32_t binary_size;
read_config_from_file(file_name, &data, &size);
- place_ptr = data;
- binsize = size;
+ binary_data = data;
+ binary_size = size;
if (!(*options).binary) {
-
if ((err = verify_xml_config(data, size)))
return LC_ERROR;
- if ((err = find_config_binary_size(data, &binsize)))
- return LC_ERROR;
-
- /* We no longer need size, let it get munged... */
- if ((err = find_config_binary_start(&place_ptr, &size)))
- return LC_ERROR;
-
- if (size < binsize)
+ if ((err = find_config_binary(data, size, &binary_data, &binary_size)))
return LC_ERROR;
if (!(*options).noweb)
- post_preconfig(data);
+ post_preconfig(data, size);
}
/*
@@ -238,14 +230,14 @@ int upload_config(char *file_name, struc
* erase the flash (to 1) in order to write the flash.
*/
printf("Erasing Flash: ");
- if ((err = erase_config(binsize, cb, (void *)0))) {
+ if ((err = erase_config(binary_size, cb, (void *)0))) {
delete_blob(data);
return err;
}
printf(" done\n");
printf("Writing Config: ");
- if ((err = write_config_to_remote(place_ptr, binsize, cb,
+ if ((err = write_config_to_remote(binary_data, binary_size, cb,
(void *)1))) {
delete_blob(data);
return err;
@@ -253,7 +245,7 @@ int upload_config(char *file_name, struc
printf(" done\n");
printf("Verifying Config:");
- if ((err = verify_remote_config(place_ptr, binsize, cb, (void *)1))) {
+ if ((err = verify_remote_config(binary_data, binary_size, cb, (void *)1))) {
delete_blob(data);
return err;
}
@@ -261,7 +253,7 @@ int upload_config(char *file_name, struc
if (!(*options).binary && !(*options).noweb) {
printf("Contacting website: ");
- if ((err = post_postconfig(data))) {
+ if ((err = post_postconfig(data, size))) {
delete_blob(data);
return err;
}
@@ -297,6 +289,7 @@ int upload_firmware(char *file_name, str
{
int err;
uint8_t *firmware;
+ uint32_t firmware_size;
uint8_t *firmware_bin;
err = 0;
@@ -322,38 +315,48 @@ int upload_firmware(char *file_name, str
}
if ((err = read_firmware_from_file(file_name, &firmware,
- (*options).binary))) {
+ &firmware_size, (*options).binary))) {
delete_blob(firmware);
return err;
}
- if ((err = extract_firmware_binary(firmware, &firmware_bin))) {
- delete_blob(firmware);
- delete_blob(firmware_bin);
- return err;
+ if ((*options).binary) {
+ firmware_bin = firmware;
+ } else {
+ if ((err = extract_firmware_binary(firmware, firmware_size, &firmware_bin))) {
+ delete_blob(firmware_bin);
+ delete_blob(firmware);
+ return err;
+ }
}
if (!(*options).direct) {
if ((err = prep_firmware())) {
printf("Failed to prepare remote for FW update\n");
+ if (firmware_bin != firmware) {
+delete_blob(firmware_bin);
+ }
delete_blob(firmware);
- delete_blob(firmware_bin);
return err;
}
}
printf("Invalidating Flash: ");
if ((err = invalidate_flash())) {
+ if (firmware_bin != firmware) {
+ delete_blob(firmware_bin);
+ }
delete_blob(firmware);
- delete_blob(firmware_bin);
return err;
}
printf(" done\n");
printf("Erasing Flash: ");
if ((err = erase_firmware((*options).direct, cb, (void *)0))) {
+ if (firmware_bin != firmware) {
+ delete_blob(firmware_bin);
+ }
delete_blob(firmware);
- delete_blob(firmware_bin);
return err;
}
printf(" done\n");
@@ -361,13 +364,18 @@ int upload_firmware(char *file_name, str
printf("Writing firmware:");
if ((err = write_firmware_to_remote(firmware_bin, (*options).direct, cb,
cb_arg))) {
+ if (firmware_bin != firmware) {
+ delete_blob(firmware_bin);
+ }
delete_blob(firmware);
return err;
}
printf(" done\n");
/* Done with this... */
- delete_blob(firmware_bin);
+ if (firmware_bin != firmware) {
+ delete_blob(firmware_bin);
+ }
if (!(*options).direct) {
if ((err = finish_firmware())) {
@@ -379,7 +387,7 @@ int upload_firmware(char *file_name, str
if (!(*options).binary && !(*options).noweb) {
printf("Contacting website:
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
On Wed, March 26, 2008 11:55 pm, Phil Dibowitz wrote: > Stephen Warren wrote: >> That was easy - I'd just accidentally included the hunk that fixed the >> C++ v.s. C comment at the end of libharmony.h, which you already >> committed separately, which caused the conflict. >> >> So, here's the updated version, attached. > > OK, some more comments. > > extract_firmware_binary() > > I thought the whole _point_ of this patch was to do this dynamically, but > yet you're still assuming FIRMWARE_SIZE This change was just to fix all the APIs for data other than the firmware blob itself. A second pass could be made to fix up all the firmware related APIs. Yes, I realize that's a tiny bit inconsistent, since I had to change extract_firmware_binary, but I only changed the XML-related parameters there. > As I mentioned, I'd like the for(;;)s to be while(1)s. OK. I'll fix that tonight. > verify_xml_config() > > You removed the size-check - why? It got moved into find_config_binary, since it's a common check for both functions. - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace ___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
Stephen Warren wrote: > That was easy - I'd just accidentally included the hunk that fixed the > C++ v.s. C comment at the end of libharmony.h, which you already > committed separately, which caused the conflict. > > So, here's the updated version, attached. OK, some more comments. extract_firmware_binary() I thought the whole _point_ of this patch was to do this dynamically, but yet you're still assuming FIRMWARE_SIZE, and you still don't pass back the firmware size... I thought we were going to make this dynamic, but check it was never larger than FIRMWWARE_SIZE which would be renamed to FIRMWARE_MAX_SIZE ? GetTag() While the separation of in/out vars isn't really consistent, it's cleaner, and provides a functionality we didn't really have, so we'll go with it. I'm still thinking we need to move to an XML library though. As I mentioned, I'd like the for(;;)s to be while(1)s. verify_xml_config() You removed the size-check - why? -- Phil Dibowitz [EMAIL PROTECTED] Open Source software and tech docsInsanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ "Never write it in C if you can do it in 'awk'; Never do it in 'awk' if 'sed' can handle it; Never use 'sed' when 'tr' can do the job; Never invoke 'tr' when 'cat' is sufficient; Avoid using 'cat' whenever possible" -- Taylor's Laws of Programming signature.asc Description: OpenPGP digital signature - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
That was easy - I'd just accidentally included the hunk that fixed the
C++ v.s. C comment at the end of libharmony.h, which you already
committed separately, which caused the conflict.
So, here's the updated version, attached.
? libconcord/.libconcord.h.swp
Index: concordance/concordance.c
===
RCS file: /cvsroot/concordance/concordance/concordance/concordance.c,v
retrieving revision 1.11
diff -d -u -r1.11 concordance.c
--- concordance/concordance.c 26 Mar 2008 01:33:31 - 1.11
+++ concordance/concordance.c 27 Mar 2008 04:22:57 -
@@ -157,7 +157,7 @@
return err;
}
- if ((err = write_config_to_file(config, file_name, size,
+ if ((err = write_config_to_file(config, size, file_name,
(*options).binary))) {
return err;
}
@@ -196,31 +196,23 @@
uint8_t *data;
uint32_t size = 0;
- uint8_t *place_ptr;
- uint32_t binsize;
+ uint8_t *binary_data;
+ uint32_t binary_size;
read_config_from_file(file_name, &data, &size);
- place_ptr = data;
- binsize = size;
+ binary_data = data;
+ binary_size = size;
if (!(*options).binary) {
-
if ((err = verify_xml_config(data, size)))
return LC_ERROR;
- if ((err = find_config_binary_size(data, &binsize)))
- return LC_ERROR;
-
- /* We no longer need size, let it get munged... */
- if ((err = find_config_binary_start(&place_ptr, &size)))
- return LC_ERROR;
-
- if (size < binsize)
+ if ((err = find_config_binary(data, size, &binary_data, &binary_size)))
return LC_ERROR;
if (!(*options).noweb)
- post_preconfig(data);
+ post_preconfig(data, size);
}
/*
@@ -238,14 +230,14 @@
* erase the flash (to 1) in order to write the flash.
*/
printf("Erasing Flash: ");
- if ((err = erase_config(binsize, cb, (void *)0))) {
+ if ((err = erase_config(binary_size, cb, (void *)0))) {
delete_blob(data);
return err;
}
printf(" done\n");
printf("Writing Config: ");
- if ((err = write_config_to_remote(place_ptr, binsize, cb,
+ if ((err = write_config_to_remote(binary_data, binary_size, cb,
(void *)1))) {
delete_blob(data);
return err;
@@ -253,7 +245,7 @@
printf(" done\n");
printf("Verifying Config:");
- if ((err = verify_remote_config(place_ptr, binsize, cb, (void *)1))) {
+ if ((err = verify_remote_config(binary_data, binary_size, cb, (void *)1))) {
delete_blob(data);
return err;
}
@@ -261,7 +253,7 @@
if (!(*options).binary && !(*options).noweb) {
printf("Contacting website: ");
- if ((err = post_postconfig(data))) {
+ if ((err = post_postconfig(data, size))) {
delete_blob(data);
return err;
}
@@ -297,6 +289,7 @@
{
int err;
uint8_t *firmware;
+ uint32_t firmware_size;
uint8_t *firmware_bin;
err = 0;
@@ -322,38 +315,48 @@
}
if ((err = read_firmware_from_file(file_name, &firmware,
- (*options).binary))) {
+ &firmware_size, (*options).binary))) {
delete_blob(firmware);
return err;
}
- if ((err = extract_firmware_binary(firmware, &firmware_bin))) {
- delete_blob(firmware);
- delete_blob(firmware_bin);
- return err;
+ if ((*options).binary) {
+ firmware_bin = firmware;
+ } else {
+ if ((err = extract_firmware_binary(firmware, firmware_size, &firmware_bin))) {
+ delete_blob(firmware_bin);
+ delete_blob(firmware);
+ return err;
+ }
}
if (!(*options).direct) {
if ((err = prep_firmware())) {
printf("Failed to prepare remote for FW update\n");
+ if (firmware_bin != firmware) {
+delete_blob(firmware_bin);
+ }
delete_blob(firmware);
- delete_blob(firmware_bin);
return err;
}
}
printf("Invalidating Flash: ");
if ((err = invalidate_flash())) {
+ if (firmware_bin != firmware) {
+ delete_blob(firmware_bin);
+ }
delete_blob(firmware);
- delete_blob(firmware_bin);
return err;
}
printf(" done\n");
printf("Erasing Flash: ");
if ((err = erase_firmware((*options).direct, cb, (void *)0))) {
+ if (firmware_bin != firmware) {
+ delete_blob(firmware_bin);
+ }
delete_blob(firmware);
- delete_blob(firmware_bin);
return err;
}
printf(" done\n");
@@ -361,13 +364,18 @@
printf("Writing firmware:");
if ((err = write_firmware_to_remote(firmware_bin, (*options).direct, cb,
cb_arg))) {
+ if (firmware_bin != firmware) {
+ delete_blob(firmware_bin);
+ }
delete_blob(firmware);
return err;
}
printf(" done\n");
/* Done with this... */
- delete_blob(firmware_bin);
+ if (firmware_bin != firmware) {
+ delete_blob(firmware_bin);
+ }
if (!(*options).direct) {
if ((err = finish_firmware())) {
@@ -379,7 +387,7 @@
if (!(*options).binary && !(*options).noweb) {
printf("Contacting website: ");
- if ((err = post_postfirmware(firmware))) {
+ if ((err = post_postfirmware(firmware, firmware_size))) {
delete_blob(firmware);
return err;
}
Index: libconcord/libconcord.cpp
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
Stephen Warren wrote: > On Wed, March 26, 2008 12:35 am, Phil Dibowitz wrote: >> Phil Dibowitz wrote: >>> Phil Dibowitz wrote: That would be great, but don't bother just yet. When I get back I'll have a more detailed look at your patch and see if there's anything else and then you can just do any needed changes at once. >>> Stephen, >>> >>> I haven't forgotten about this patch - but I'm out of >>> catch-up-on-concordance time today (this is the only thing pending), I >>> should get to this tomorrow. >> Actually - it doesn't apply against the latest CVS - I could massage >> it in, but if you'd be willing to re-diff it against the latest CVS, >> it'd make my life easier... > > Sure. Hopefully tonight. Previously, you'd mentioned possibly wanting some > changes to the patch too; do you still want any? Well, I was going to look at the actual changed to as a final run-through, but it didn't apply, and I had to run, so I didn't... so other than the stuff I mentioned previously, I know of no new things, but there were a few places I wanted to look at the resultant code. -- Phil Dibowitz [EMAIL PROTECTED] Open Source software and tech docsInsanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ "Never write it in C if you can do it in 'awk'; Never do it in 'awk' if 'sed' can handle it; Never use 'sed' when 'tr' can do the job; Never invoke 'tr' when 'cat' is sufficient; Avoid using 'cat' whenever possible" -- Taylor's Laws of Programming signature.asc Description: OpenPGP digital signature - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
On Wed, March 26, 2008 12:35 am, Phil Dibowitz wrote: > Phil Dibowitz wrote: >> Phil Dibowitz wrote: >>> That would be great, but don't bother just yet. When I get back >>> I'll have a more detailed look at your patch and see if there's >>> anything else and then you can just do any needed changes at >>> once. >> >> Stephen, >> >> I haven't forgotten about this patch - but I'm out of >> catch-up-on-concordance time today (this is the only thing pending), I >> should get to this tomorrow. > > Actually - it doesn't apply against the latest CVS - I could massage > it in, but if you'd be willing to re-diff it against the latest CVS, > it'd make my life easier... Sure. Hopefully tonight. Previously, you'd mentioned possibly wanting some changes to the patch too; do you still want any? - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace ___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
Phil Dibowitz wrote: > Phil Dibowitz wrote: >> That would be great, but don't bother just yet. When I get back I'll have a >> more detailed look at your patch and see if there's anything else and then >> you >> can just do any needed changes at once. > > Stephen, > > I haven't forgotten about this patch - but I'm out of > catch-up-on-concordance time today (this is the only thing pending), I > should get to this tomorrow. Actually - it doesn't apply against the latest CVS - I could massage it in, but if you'd be willing to re-diff it against the latest CVS, it'd make my life easier... -- Phil Dibowitz [EMAIL PROTECTED] Open Source software and tech docsInsanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ "Never write it in C if you can do it in 'awk'; Never do it in 'awk' if 'sed' can handle it; Never use 'sed' when 'tr' can do the job; Never invoke 'tr' when 'cat' is sufficient; Avoid using 'cat' whenever possible" -- Taylor's Laws of Programming signature.asc Description: OpenPGP digital signature - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
Phil Dibowitz wrote: > That would be great, but don't bother just yet. When I get back I'll have a > more detailed look at your patch and see if there's anything else and then you > can just do any needed changes at once. Stephen, I haven't forgotten about this patch - but I'm out of catch-up-on-concordance time today (this is the only thing pending), I should get to this tomorrow. -- Phil Dibowitz [EMAIL PROTECTED] Open Source software and tech docsInsanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ "Never write it in C if you can do it in 'awk'; Never do it in 'awk' if 'sed' can handle it; Never use 'sed' when 'tr' can do the job; Never invoke 'tr' when 'cat' is sufficient; Avoid using 'cat' whenever possible" -- Taylor's Laws of Programming signature.asc Description: OpenPGP digital signature - Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
On Sun, Mar 16, 2008 at 10:43:07PM -0600, Stephen Warren wrote: > Phil Dibowitz wrote: > > On Sun, Mar 16, 2008 at 02:25:42PM -0600, Stephen Warren wrote: > >> GetTag: Add size parameter; the point of the change. > > > > Reading through the diff it looks like the user passes in two pointers here > > - one that's the start of the data and one that's the modified to the start > > of > > the found tag. The user has to make a second pointer anyway, so we might as > > well just let them make that temp pointer be the start of the data as we > > were > > doing before... don't see the need to add another thing in the stack. > > As a personal preference, I like to avoid inout parameters, such that > all parameters are either parameters passed in to, or results passed out > of, a function. A result of this is that at the call site, "x" is in, > and "&x" is out; a /little/ more self-documenting without reading the > function docs. > > Still, I can change that if it really bugs you. Honestly, I tend to code things this way as well, but I'm also trying to keep the codebase consistent, and I think we have other in/out vars scattered around. > Either way works fine for me; I just worked in environments (peer > groups) that preferred for(;;) "for ever". I can certainly switch the > over though. That would be great, but don't bother just yet. When I get back I'll have a more detailed look at your patch and see if there's anything else and then you can just do any needed changes at once. -- Phil Dibowitz [EMAIL PROTECTED] Open Source software and tech docsInsanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ "Never write it in C if you can do it in 'awk'; Never do it in 'awk' if 'sed' can handle it; Never use 'sed' when 'tr' can do the job; Never invoke 'tr' when 'cat' is sufficient; Avoid using 'cat' whenever possible" -- Taylor's Laws of Programming signature.asc Description: Digital signature - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/___ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
Phil Dibowitz wrote:
> On Sun, Mar 16, 2008 at 02:25:42PM -0600, Stephen Warren wrote:
>> GetTag: Add size parameter; the point of the change.
>
> Reading through the diff it looks like the user passes in two pointers here
> - one that's the start of the data and one that's the modified to the start of
> the found tag. The user has to make a second pointer anyway, so we might as
> well just let them make that temp pointer be the start of the data as we were
> doing before... don't see the need to add another thing in the stack.
As a personal preference, I like to avoid inout parameters, such that
all parameters are either parameters passed in to, or results passed out
of, a function. A result of this is that at the call site, "x" is in,
and "&x" is out; a /little/ more self-documenting without reading the
function docs.
Still, I can change that if it really bugs you.
> The one thing that stood out:
>
>> +// Consume tags until there aren't any left
>> +for (;;) {
>> +// Loop searching for start of tag character
>> +for (;;) {
>
> Nitpicky, but stylistically I'd prefer while (1) here. I was always taught
> that for is for when you know how many interations (foo < bar) and while is
> for when you don't (1, or "still exists" or read() or whatever). I'm guessing
> your not a big fan of do-while either - neither am I. I didn't write those :)
Either way works fine for me; I just worked in environments (peer
groups) that preferred for(;;) "for ever". I can certainly switch the
over though.
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
Re: [concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
On Sun, Mar 16, 2008 at 02:25:42PM -0600, Stephen Warren wrote:
> Attached is a patch that adds a "size" parameter to all APIs that
> manipulate variable-sized XML configuration data.
Very nice Stephen. A few comments.
> find_config_binary*: Merged these 2 functions into 1, which also
> performs the appropriate error-checking. This "lifts" the error checking
> logic into libconcord, so applications don't have to know how to do it.
> The new API also returns both location and size of the binary data, for
> passing to all the other APIs.
This is really elegant, nice.
> GetTag: Add size parameter; the point of the change.
Reading through the diff it looks like the user passes in two pointers here
- one that's the start of the data and one that's the modified to the start of
the found tag. The user has to make a second pointer anyway, so we might as
well just let them make that temp pointer be the start of the data as we were
doing before... don't see the need to add another thing in the stack.
To be clear, I'm totally against over-optimzation - I'll take 20 things on the
stack if it makes the code cleaner or easier to maintain, but I don't see the
benefit here.
> concordance: Binary firmware writes: Don't extract_firmware_binary, nor
> free the not-separately-allocated firmware_bin, when in binary mode.
Haha! That was a gaping bug. Thanks.
> verify_xml_config: "Lift" validation of binary size into
> find_config_binary, and re-use that validation to avoid duplication of
> logic.
Nice.
> Things remaining:
>
> I wonder if we should do this: Everywhere that uses GetTag currently
> searches the entire XML file for the tag. I wonder if we shouldn't
> always search for , then only search *before* that point,
> just to make sure we don't accidentally find something within the binary
> portion. Such a change would probably be purely internal to the
> functions though, so no API changes would be required (unless we want to
> perhaps pre-parse this bit during file read, and return a structure with
> ptr/size/bin_ptr/bin_size to clients...)
I'm wondering if we shouldn't just find a light-weight XML library and be done
with it. Parsing this XML ourselves is kinda ghetto.
> Index: libconcord/web.cpp
I've been meaning to ask... shouldn't GetTag() be in binaryfile.cpp?!
I need to spend some more time looking through the diff, and also looking at
the resulting code as well, but I did read through it and it looks pretty
solid. The one thing that stood out:
> + // Consume tags until there aren't any left
> + for (;;) {
> + // Loop searching for start of tag character
> + for (;;) {
Nitpicky, but stylistically I'd prefer while (1) here. I was always taught
that for is for when you know how many interations (foo < bar) and while is
for when you don't (1, or "still exists" or read() or whatever). I'm guessing
your not a big fan of do-while either - neither am I. I didn't write those :)
Thanks for doing this!
--
Phil Dibowitz [EMAIL PROTECTED]
Open Source software and tech docsInsanity Palace of Metallica
http://www.phildev.net/ http://www.ipom.com/
"Never write it in C if you can do it in 'awk';
Never do it in 'awk' if 'sed' can handle it;
Never use 'sed' when 'tr' can do the job;
Never invoke 'tr' when 'cat' is sufficient;
Avoid using 'cat' whenever possible" -- Taylor's Laws of Programming
signature.asc
Description: Digital signature
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/___
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
[concordance-devel] Patch: Add "size" parameter to all non-firmware APIs
Attached is a patch that adds a "size" parameter to all APIs that
manipulate variable-sized XML configuration data.
I have tested *all* operations that the concordance application exposes,
in particular, read/write config/firmware in xml/binary mode.
The diff looks a little scarier than I feel it is; perhaps apply it then
compare the code visually...
Explanation of API changes:
delete_blob: I figured this didn't need a return code. If it does,
revert the .h change, but I figure it should return 0, not 1, to be
consistent with other APIs.
post_*, verify_xml_config: Add size parameter to allow implementation
not to parse beyond the end of the XML data.
write_config_to_file: Simply re-ordered the parameters so that size was
immediately after the pointer that it's the size of, to be consistent
with all other APIs accepting a size parameter.
find_config_binary*: Merged these 2 functions into 1, which also
performs the appropriate error-checking. This "lifts" the error checking
logic into libconcord, so applications don't have to know how to do it.
The new API also returns both location and size of the binary data, for
passing to all the other APIs.
read_firmware_from_file: I wasn't going to touch any of the firmware
APIs, but since this one can return XML data, it also needs to return
the size of the XML data just like read_config_from_file.
extract_firmware_binary: Again, this can parse XML, so it needs a size
parameter.
GetTag: Add size parameter; the point of the change.
Post: Add size parameter to pass to GetTag.
Implementation notes, other than simply reacting to API changes:
concordance: Binary firmware writes: Don't extract_firmware_binary, nor
free the not-separately-allocated firmware_bin, when in binary mode.
verify_xml_config: "Lift" validation of binary size into
find_config_binary, and re-use that validation to avoid duplication of
logic.
extract_firmware_binary: Validate there isn't more firmware in the XML
file than we expect, to avoid writing past end of allocated data.
All XML file reading: Allocate arrays as exactly the size we want to
read, and don't put a "NULL" character after the read data, because now
we pass the size everywhere.
Post (and a few other places): When calling GetTag repeatedly, and
starting the search from where the last one left off, we need to
re-calculate the size of data left each time "xml_size-(x-xml)".
GetTag: Ended up re-writing this quite a bit to make use of the new size
parameter. Hopefully the comments explain how this works pretty well.
I think that's it...
Things remaining:
I wonder if we should do this: Everywhere that uses GetTag currently
searches the entire XML file for the tag. I wonder if we shouldn't
always search for , then only search *before* that point,
just to make sure we don't accidentally find something within the binary
portion. Such a change would probably be purely internal to the
functions though, so no API changes would be required (unless we want to
perhaps pre-parse this bit during file read, and return a structure with
ptr/size/bin_ptr/bin_size to clients...)
? concordance/.concordance.c.swp
? concordance/.deps
? concordance/.libs
? concordance/Makefile
? concordance/Makefile.in
? concordance/aclocal.m4
? concordance/autom4te.cache
? concordance/concordance
? concordance/config.guess
? concordance/config.h
? concordance/config.h.in
? concordance/config.log
? concordance/config.status
? concordance/config.sub
? concordance/configure
? concordance/depcomp
? concordance/install-sh
? concordance/libtool
? concordance/ltmain.sh
? concordance/missing
? concordance/stamp-h1
? consnoop/snoop
? libconcord/.deps
? libconcord/.libconcord.cpp.swp
? libconcord/.libconcord.h.swp
? libconcord/.libs
? libconcord/.web.h.swp
? libconcord/Makefile
? libconcord/Makefile.in
? libconcord/aclocal.m4
? libconcord/autom4te.cache
? libconcord/binaryfile.lo
? libconcord/config.guess
? libconcord/config.h
? libconcord/config.h.in
? libconcord/config.log
? libconcord/config.status
? libconcord/config.sub
? libconcord/configure
? libconcord/depcomp
? libconcord/install-sh
? libconcord/libconcord.la
? libconcord/libconcord.lo
? libconcord/libtool
? libconcord/libusbhid.lo
? libconcord/ltmain.sh
? libconcord/missing
? libconcord/remote.lo
? libconcord/remote_z.lo
? libconcord/stamp-h1
? libconcord/usblan.lo
? libconcord/web.lo
Index: concordance/concordance.c
===
RCS file: /cvsroot/concordance/concordance/concordance/concordance.c,v
retrieving revision 1.10
diff -u -r1.10 concordance.c
--- concordance/concordance.c 16 Mar 2008 07:00:52 - 1.10
+++ concordance/concordance.c 16 Mar 2008 20:05:11 -
@@ -157,7 +157,7 @@
return err;
}
- if ((err = write_config_to_file(config, file_name, size,
+ if ((err = write_config_to_file(config, size, file_name,
(*options).binary))) {
return err;
}
@@ -196,31 +196,23 @@
uint8_t *data;
uint32_t size = 0;
- uint8_t *place_ptr;
