On Sun, Nov 13, 2011 at 3:50 PM, Michael Shields <span dir="ltr"><<a href="mailto:shields@msrl.com">shields@msrl.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">On Sat, Nov 12, 2011 at 9:37 PM, Jonathan Lassoff <<a href="mailto:jof@thejof.com">jof@thejof.com</a>> wrote:<br>
><br>
> This is my hack job: <a href="https://gist.github.com/1361655" target="_blank">https://gist.github.com/1361655</a><br>
> I'm still a little new to writing C from scratch (I'm usually modifying,<br>
> porting, or fixing stuff), so I'd appreciate any feedback on this. It<br>
> compiles and works on my machine, but that's the extent of my testing -- I<br>
> still haven't tested it out at 2169.<br>
<br>
</div>This code looks pretty good overall.<br>
<br>
It has a bug: if the packet received is larger than 255 bytes, the<br>
buffer won't be NUL-terminated, so strncmp will read past the end of<br>
it. You can either check for this case, explicitly set the last byte<br>
of command_buffer to NUL, or use memcmp.<br></blockquote><div><br></div><div>This was why I tried using strncmp and the sizeof of the largest command first so that it would stop comparing once it's read as much as it could possibly need. This probably needs a comment.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
is_buzzer_ringing() can return -1, but its only caller doesn't check for this.<br></blockquote><div><br></div><div>Gotcha.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
You'll want to run it through gcc -Wall, which has a few suggestions:<br>
<br>
parport_daemon.c: In function ‘is_buzzer_ringing’:<br>
parport_daemon.c:76:3: warning: suggest parentheses around<br>
comparison in operand of ‘&’ [-Wparentheses]<br>
<br>
This is a style issue but I agree. Using & instead of && and =<br>
instead of == in a condition is such a common bug that there needs to<br>
be some way to indicate that you know what you're doing.<br></blockquote><div><br></div><div>Makes sense.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
parport_daemon.c: In function ‘main’:<br>
parport_daemon.c:222:7: warning: implicit declaration of function<br>
‘strncmp’ [-Wimplicit-function-declaration]<br>
<br>
Fix this with #include <string.h>.<br></blockquote><div><br></div><div>Gah. Why is there both string.h and strings.h? </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
parport_daemon.c:226:66: warning: passing argument 4 of<br>
‘send_response’ from incompatible pointer type [enabled by default]<br>
parport_daemon.c:58:5: note: expected ‘char *’ but argument is of<br>
type ‘char (*)[31]’<br>
parport_daemon.c:231:66: warning: passing argument 4 of<br>
‘send_response’ from incompatible pointer type [enabled by default]<br>
parport_daemon.c:58:5: note: expected ‘char *’ but argument is of<br>
type ‘char (*)[25]’<br>
parport_daemon.c:239:66: warning: passing argument 4 of<br>
‘send_response’ from incompatible pointer type [enabled by default]<br>
parport_daemon.c:58:5: note: expected ‘char *’ but argument is of<br>
type ‘char (*)[6]’<br>
parport_daemon.c:244:66: warning: passing argument 4 of<br>
‘send_response’ from incompatible pointer type [enabled by default]<br>
parport_daemon.c:58:5: note: expected ‘char *’ but argument is of<br>
type ‘char (*)[9]’<br>
<br>
You mean to use r_acknowleged instead of &r_acknowledged, etc. Or<br>
&r_acknowleged[0] if you prefer.<br></blockquote><div><br></div><div>Good point. That's a much simpler way to pointing at the beginning of an array.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
By the way, you might well make these const.<br>
<br>
parport_daemon.c:168:9: warning: unused variable ‘childpid’<br>
[-Wunused-variable]<br>
<br>
Remove.<br>
<br>
parport_daemon.c: In function ‘send_response’:<br>
parport_daemon.c:65:1: warning: control reaches end of non-void<br>
function [-Wreturn-type]<br>
<br>
Return 0 at the end.<br>
<br>
The macros on lines 37-42 aren't used and can be removed.<br>
<br>
ringer_state and buzzer_state can be bools. You might also want to<br>
give them names like recently_buzzed. If they could have more than<br>
two states, an enum would be a good choice.<br></blockquote><div><br></div><div>All very good ideas.</div><div><br></div><div>-Wall, good idea. Thanks for the kind feedback.</div><div><br></div><div>Cheers,</div><div>jof </div>
</div><br>