[Dillo-dev]HTTPS Cleanup
I have gone through and modified the https.c file to use (what I believe are) your preferred coding standards, with one exception. I *cannot* stand unbraced if statements (or any other statements), and thus I braced them in two locations. I also don't care for use of the tertiary operator, but you wrote the function it is used in, so I don't plan on touching it. I would also like your opinion on how to "clean up" the function save_certificate_home() It has three exit points, and dropping it to fewer would be somewhat of a challenge to do cleanly. There are a few loctions where I believe you broke a statement which would ift on one line, specifically snprintf statements. I might have dont this (though I can't think of why). If this is the case, could you let me know your reasoning behind this? I'd like to learn from this if possible... - Garrett Kajmowicz
On Fri, Jul 23, 2004 at 01:10:42PM -0400, Garrett Kajmowicz wrote:
I have gone through and modified the https.c file to use (what I believe are) your preferred coding standards, with one exception.
Done. (i.e. commited).
I *cannot* stand unbraced if statements (or any other statements), and thus I braced them in two locations. I also don't care for use of the tertiary operator, but you wrote the function it is used in, so I don't plan on touching it.
No problem. Always-braced if statements probably is a better practice!
I would also like your opinion on how to "clean up" the function save_certificate_home() It has three exit points, and dropping it to fewer would be somewhat of a challenge to do cleanly.
OK, I'll look at it.
There are a few loctions where I believe you broke a statement which would ift on one line, specifically snprintf statements. I might have dont this (though I can't think of why). If this is the case, could you let me know your reasoning behind this? I'd like to learn from this if possible...
Those statements were originally in your patch! :-) <q> [...] + /*Attempt to create .dillo/certs blindly - check later*/ + snprintf(buf,4096,"%s/.dillo/", + g_get_home_dir() + ); + mkdir(buf, 01777); + snprintf(buf,4096,"%s/.dillo/certs/", + g_get_home_dir() + ); + mkdir(buf, 01777); + + do{ + snprintf(buf,4096,"%s/.dillo/certs/%x.%u\0", + g_get_home_dir(), + X509_subject_name_hash(cert), i + ); [...] </q> You can make them one line. --- BTW, g_free() tests for NULL so this statements: if (http_query != NULL){ g_free(http_query); http_query = NULL; } can simply become: g_free(http_query); because the variable is local, there's no need to NULL it. Except for 'root_url' which can also be made local. Finally, please send the patches to me directly (not to the list), but comments belong to the list. This saves some band width to hundreds of subscribers. Thanks for taking my advice. It'so much easier to review the patches now. Cheers Jorge.-
On Fri, Jul 23, 2004 at 01:10:42PM -0400, Garrett Kajmowicz wrote:
[...] I would also like your opinion on how to "clean up" the function save_certificate_home() It has three exit points, and dropping it to fewer would be somewhat of a challenge to do cleanly.
Done. I also added some comments, and put the snprintf's in one line. BTW, why 01777 instead of 01700? Cheers Jorge.-
On July 23, 2004 07:33 pm, Jorge Arellano Cid wrote:
On Fri, Jul 23, 2004 at 01:10:42PM -0400, Garrett Kajmowicz wrote:
[...] I would also like your opinion on how to "clean up" the function save_certificate_home() It has three exit points, and dropping it to fewer would be somewhat of a challenge to do cleanly.
Done.
I also added some comments, and put the snprintf's in one line.
BTW, why 01777 instead of 01700?
The flags setting are automatically bitwize-anded with the umask. My thinking at the time was that it would be better to have the user settings determine how the certificates would be saved. And since the only thing that certificate access might give people is an extremely small list of sites the user had been to, I didn't deem it a security risk. If you prefer I change it, I will do so.... - Garrett Kajmowicz
participants (2)
-
Garrett Kajmowicz -
Jorge Arellano Cid