On Sat, 21 Sep 2013, Phil Dibowitz wrote:
>> +int _mh_write_config_to_file(uint8_t *in, uint32_t size, char *file_name)
>> +{
>> + int zip_err;
>> + struct zip *zip = zip_open(file_name, ZIP_CREATE | ZIP_EXCL, &zip_err);
>> + if (!zip) {
>> + if (zip_err == ZIP_ER_EXISTS) {
>> + printf("Error: file %s already exists\n", file_name);
>> + } else {
>> + debug("Failed to create zip file %s", file_name);
>
> Do you want to use `zip_error_to_str` here?
Sure, why not. I think I'll leave the printf for the one error
because I want regular users to be aware of that one. I'm not sure
what other errors are really possible on a create, but I'll go ahead and
add a call to zip_error_to_str() in the next rev.
>> + }
>> + return LC_ERROR_OS_FILE;
>> + }
>> + int index;
>> +
>> + // Write XML
>> + extern const char *mh_config_header;
>> + int xml_buffer_len = strlen(mh_config_header) + 100;
>
> Why 100? Is that just a guess for the extra data your snprintf()ing in? is it
> exact, or a guess? can you comment this?
It's just an guess (with a lot of extra added in just in case) for the
data we're snprintf()ing. I added a comment.
>> + char *xml_buffer = new char[xml_buffer_len];
>> + uint16_t checksum = mh_get_checksum(in, size);
>> + int xml_len = snprintf(xml_buffer, xml_buffer_len, mh_config_header,
>> + size, size - 6, checksum, ri.skin);
>> + if (xml_len >= xml_buffer_len) {
>> + debug("Error, XML buffer length exceeded");
>> + return LC_ERROR;
>> + }
>> + struct zip_source *xml = zip_source_buffer(zip, xml_buffer, xml_len, 0);
>
> why not use freep and let it free this on it's own?
What's freep?
>> + if (!xml) {
>> + debug("Failed to create zip_source_buffer for XML file");
>> + return LC_ERROR_OS_FILE;
>> + }
>> + index = zip_add(zip, "Description.xml", xml);
>
> According to the docs:
>
> The function zip_add() is the obsolete version of zip_file_add(3).
Weird, zip_file_add doesn't even exist in the version of libzip I'm using
(on Fedora):
[talbert@easel libconcord]$ grep add /usr/include/zip.h
ZIP_EXTERN zip_int64_t zip_add(struct zip *, const char *, struct zip_source *);
ZIP_EXTERN zip_int64_t zip_add_dir(struct zip *, const char *);
It looks like zip_file_add() must've just been added in the most recent
release. I figure we should probably stick with the older function until
the new one has been out for longer.
>> + if (index == -1) {
>> + debug("Error writing XML to zip file");
>> + zip_source_free(xml);
>> + return LC_ERROR_OS_FILE;
>> + }
>> +
>> + // Write EzHex file
>> + struct zip_source *ezhex = zip_source_buffer(zip, in, size, 0);
>> + if (!ezhex) {
>> + debug("Failed to create zip_source_buffer for EzHex file");
>> + return LC_ERROR_OS_FILE;
>> + }
>> + index = zip_add(zip, "Result.EzHex", ezhex);
>> + if (index == -1) {
>> + debug("Error writing EzHex to zip file");
>> + zip_source_free(ezhex);
>> + return LC_ERROR_OS_FILE;
>> + }
>
> You have a leak here... you only ever free 'xml' zip_buffer on error, so in
> this case you return and free ezhex, but NOT xml.
Actually, you don't free zip_source's if you successfully feed them into a
zip_add (it consumes them). See the man page for zip_source_free. So, no
leak here, but I did actually have a leak with xml_buffer, which I've
corrected.
>> + struct zip *zip = zip_open(file_name, 0, NULL);
>> + if (zip) {
>
> The code gets a lot cleaner here if you change this to:
>
> if (!zip) {
> // error
> }
> //code
>
> I know - you were just keeping it as it was, but it was worth pointing out.
Not my code, but I'll happily clean it up for you in the next rev. :-)
>> + /* reset the sequence number to 0 */
>> + const uint8_t msg_reset_seq[MH_MAX_PACKET_SIZE] =
>
> We do this a lot - perhaps it should be abstracted into a function?
Seems reasonable - went ahead and did so in rev 5.
>> +// Calculates the XOR checksum for a config read from the remote.
>> +uint16_t mh_get_checksum(uint8_t* rd, const uint32_t len)
>> +{
>> + // This is the "SEED" that all the configs from the website use.
>> + uint16_t cksum = 0x4321;
>> + // The part of the config that gets checksummed is consistently 6 bytes
>> + // less than the length of the config. Since we are checksumming two
>> + // bytes at a time, we stop when i == len - 7, which is the same as
>> + // i + 1 == len - 6. In the case of odd lengths, we skip the last byte.
>> + for (int i = 0; i < (len - 7); i += 2) {
>> + uint16_t j = (rd[i+1] << 8) + rd[i];
>> + cksum ^= j;
>> + }
>> + debug("CHECKSUM=0x%04x", cksum);
>> + return cksum;
>> +}
>
> How did you figure that out?
*LOTS* and *LOTS* of trial and error. :-)
>> + /*
>> + * There are four extra bytes at the end of every config file, but they
>> + * are not present when read from the remote. Add them here.
>> + */
>
> Wait - but mh_find_eof says that you often get those 4 bytes plus a extra
> padding?
See, this is why it is bad for us to let these patches fester for months.
I completely forget what the heck I was thinking! Now that I've refreshed
my memory, the issue is that reading the config from the remote, some
remotes (the 200) return the config, then the EOF bytes, then a bunch of
junk/padding. On the other hand, the 300 returns just the config (no EOF,
no padding). My solution to that was to just always add the EOF bytes at
the end of the buffer - that way, when I check for the config length
later, I'll either find the 200's real EOF, or in the case of the 300, the
fake EOF that I added and get the correct config length in either case.
I'll update the comments to make this clearer.
Scott
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk
_______________________________________________
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
