Stephen Warren wrote: > This patch adds a size parameter to any/all public APIs where it makes > sense. I believe it's complete.
Mmm, I'm really starting to like this patch. Now that I see the code, combining verify_xml_config and find_config_binary was definitely the right choice. One comment on ** vs *&. The code tends to use *& in any non-public API, and ** only for public functions since C requires that. You converted various private APIs to **, and I prefer to leave them *& to be consistent with the entire rest of the codebase. Only the actual libconcord.h API gets **, and that header file is in fact the dividing line. So I'd like that reversed. If you find it necessary, you can add a comment somewhere to that effect, perhaps in libconcord.cpp... > erase_safemode, erase_firmware: Add size parameter (app is expected to > pass size from other APIs back here), and validate it's small enough to > fit into the firmware area. But, always erase the whole area to ensure > the flash is clean. I don't think erase_safemode() should take a size. It really doesn't care about size. erase_safemode shouldn't assume a write_safemode() is next, and size makes no sense to it. The documentation for this API should simply say "the entire firmware area will be erased and available for writing". > read_safemode_from_remote, read_firmware_from_remote: Add size output > parameter. Hard-code result to FIRMWARE_SIZE, since that's all we know > on reads. This constant no longer really means FIRMWARE_SIZE within the bounds of your patch, so lets rename this FIRMWARE_MAX_SIZE, if you don't mind. > write_firmware_to_file: See notes above. Also, don't assume that size is > a multiple of 32 when converting data to hex. Also, move debug checksum > calculation into #ifdef _DEBUG. Wow, that is kinda of a useless calculation in there. Nice call. > convert_to_binary: Convert ptr to ** from *& for consistency with other > APIs in libconcord.cpp. See above. Other than that, I don't see anything else that concerns me. Sorry to keep going around so many times, but it's a fairly invasive patch, so I'm just trying to make sure we have everything so when it merges, it stays. -- Phil Dibowitz [EMAIL PROTECTED] Open Source software and tech docs Insanity 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
