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 </INFORMATION>, 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 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: Digital signature
------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
