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
