<!DOCTYPE html>
<html>
<head>
<title></title>
</head>
<body><div style="font-family:Arial;">I don’t know why you still put the loop counter in fec_xor at the function body’s scope, but other than that, I guess I’m fine with the code now. The check for 'f2->i_buffer != 0' is rather indirect and well, the whole function is not written in my style, but it doesn’t seem like my style is liked by many, either. ;) The comments are probably OK.<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">I noticed that you added the variable names to the function<br></div>
<div style="font-family:Arial;">declarations, as requested. Thanks.<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">> if (unlikely(htons(new_hdr->size) != (new->i_buffer - sizeof(struct<br></div>
<div style="font-family:Arial;">> dozer_hdr)))) {<br></div>
<div style="font-family:Arial;">> msg_Err(access, "Dozer Recovery FAILED. Invalid size %u for<br></div>
<div style="font-family:Arial;">> recovered packet.",<br></div>
<div style="font-family:Arial;">> ntohs(new_hdr->size));<br></div>
<div style="font-family:Arial;">I told you the BE protocol definition was going to cause problems … :P<br></div>
<div style="font-family:Arial;">(htons instead of ntohs)<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;"><br></div>
<blockquote><div style="font-family:Arial;">msg_Dbg(access, "Dozer Packet Recovered. Cur win: %u idx: %u buffer size: %lu size \<br></div>
<div style="font-family:Arial;"> stored: %u", ntohs(win), new_idx, new->i_buffer, ntohs(new_hdr->size));<br></div>
</blockquote><div style="font-family:Arial;">This one looks bogous as well, why should 'win' have to be converted? (Line 425)<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">> if ((new_idx == sys->fec_last_rx + 1U)) {<br></div>
<div style="font-family:Arial;">> sys->fec_last_rx++;<br></div>
<div style="font-family:Arial;">> }<br></div>
<div style="font-family:Arial;">><br></div>
<div style="font-family:Arial;">> while (sys->fec_last_rx < (sys->n_tot - 1) && (sys->matrix[sys->fec_last_rx + 1])) {<br></div>
<div style="font-family:Arial;">> sys->fec_last_rx++;<br></div>
<div style="font-family:Arial;">> }<br></div>
<div style="font-family:Arial;">I assume you’re doing this to keep track of how many packets have<br></div>
<div style="font-family:Arial;">already been registered, but could you please state this definitely in<br></div>
<div style="font-family:Arial;">the source?<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">fec_rebuild_packet still has the variable 'i' placed at the function body scope …<br></div>
<div style="font-family:Arial;">fec_try_recovery is unchanged?<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">Semantically, it doesn’t make sense to call a function with the name fec_try_recovery from a function that pretends to initialize FEC stuff (process_fec_initials ->*), some name adjustments (or even refactorings) might be needed. (*-> This name is also bad. I realize English might not be the language you’re the most proficient in (nor is it mine), but that name just doesn’t really seem to make sense. It’s not totally off, it does convey the notions that something is changed in the packet, that it has something to do with FEC and that it is to be done at the beginning, but, it’s just fuzzy and not quite on the point. I hope you understand. It’s the case for a few other names, too. It really doesn’t help that the function will do different things based on external state – it will either just fill in the counts or it will allocate memory and initialize it in a specific way. I couldn’t come up with any better one after a few tries, though.)<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">Well, I don’t feel like continuing now (there have already been two or three days between starting to write this mail (the top part) and sending it ;) ), but honestly, scrolling through it, it doesn’t seem like you have changed any other parts of the code I had looked at, anyways. (No offense on my part, although given the two NAKs that have arrived in the meantime, it doesn’t seem like you’ll be able to dodge improving those pieces of code :P )<br></div>
<pre>
--
http://www.fastmail.com - The professional email service
</pre>
</body>
</html>