Phil Dibowitz wrote:
> Stephen Warren wrote:
>> 1) Ensure all libconcord.h APIs either take in (or send back) a length
>> parameter for all buffers read or created. Most do, a couple don't.
>
> I thought went through and added lengths anywhere that made sense - did I
> miss places? Could you give me an example?
The following should be updated:
(I think I missed the first 3 on my previous round, and the others were
possibly added later.)
int find_binary_size(uint8_t *ptr, uint32_t *size);
int post_preconfig(uint8_t *data);
int post_postconfig(uint8_t *data);
int read_safemode_from_remote(uint8_t **out, lh_callback cb,
void *cb_arg);
int write_safemode_to_file(uint8_t *in, char *file_name);
int read_safemode_from_file(char *file_name, uint8_t **out);
int read_firmware_from_remote(uint8_t **out, lh_callback cb,
void *cb_arg);
int write_firmware_to_remote(uint8_t *in, int direct, lh_callback cb,
void *cb_arg);
int write_firmware_to_file(uint8_t *in, char *file_name, int binary);
int read_firmware_from_file(char *file_name, uint8_t **out, int binary);
>> 2) Pass buffer size into GetTag, so it doesn't assume the input is
>> null-terminated. (There's definitely at least one bug in GetTag right
>> now...)
>
> Heh - I really hate that code. So, here's a few thoughts I have on this:
>
> - Null-terminated is actually fine if it was *NULL* terminated not 0
> terminated. I'd like to actually change that to be null-terminated.
??? 0-terminated and NULL-terminated are the same thing. Also, don't
confuse NULL termination (0-valued character value) with the NULL
pointer (0 value of pointer type).
And incidentally, on a related note, since this is C++ code, 0 and NULL
are defined (by C++ standard) as the same thing to (for pointer values).
> - We're reading into a big array now, which is a bit silly when we could
> mmap() the file instead. That would be much more efficient, and that's on
> my list of things to fix... though I'd gladly accept a patch.
Well, I guess we could mmap. That might make the code less portable if
the API requires a mmap-style implementation, rather than simply allows
it. That said, this detail can probably be hidden - we'd either have
read_data_from_file -> new pointer
free_buffer -> delete pointer
read_data_from_file -> mmap pointer
free_buffer -> munmap
If the API did allocate anything internally (e.g. when reading from the
remote) we'd need to make sure there was a separate free_buffer_xxx API
for each case, so they wouldn't get confused.
>> 4) Add a function to free buffers, so non-C++ clients can effectively
>> call delete[] through the libconcord API for the buffers they got back.
>
> I actually started on this somewhere based on your comment before but got
> distracted. This definitely needs to happen before the next release. You
> *can* do it, but I think I have a directory somewhere with it done or almost
> done and need to commit it... so feel free leave this one to me if you'd like.
Assuming no allowance for mmap, it's just one function that calls
delete[]. So, I'll probably put it in, and we can augment it if/when
mmap is implemented...
-------------------------------------------------------------------------
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
