Alex Bennee wrote: > It needs a quick tidy up before I submit the final patch.
Here we go...
> --- src/core/bitzi.c 21 Mar 2005 22:08:56 -0000 1.13
> +++ src/core/bitzi.c 6 Apr 2005 08:06:07 -0000
> @@ -39,9 +39,11 @@
>
> #include <libxml/parser.h>
> #include <libxml/tree.h>
> +#include <stdio.h> /* rename() */
What do you need this for? It's included in common.h.
> @@ -264,6 +268,20 @@
> gchar *s = NULL;
Drop the initialization. It'll just hide a useful compile-time warning
if you forget to initialize it.
> /*
> + * We extract the urn:sha1 from the ticket as we may be processing
> + * cached tickets not associated with any actual request
> + */
> + s = xml_get_string(node, "about");
> + if (s) {
> + const gchar *urnsha1;
> + urnsha1 = base32_sha1(&s[9]);
The "9" is obvious to you, me and a few others but you should really
refer to "urn:sha1:" instead. base32_sha1() had a bug in the previous
revision. However, you really need to check the input here. Bitzi
could send you whatever and if it isn't an urn:sha1 it'll just crash.
> + data->urnsha1 = atom_sha1_get(urnsha1);
> + /*
> + * If the data has a valid date then re-echo the XML ticket
> + * to the file based cache.
> + */
>
> + now = time(NULL);
I haven't looked at the surrounding code...but - if possible - propagate
the current time from a higher call because time() is "somewhat" heavy
due to being a wrapper around a system call.
> +
> + if (now<data->expiry)
This has really no defined meaning with respect to portability. Use
delta_time() instead.
> @@ -689,8 +729,105 @@
> void
> bitzi_init(void)
> {
> + gchar *path = NULL, *oldpath = NULL;
Same as above. Don't pre-initialize the variables.
> + /*
> + * Rename the old file , overwritting stuff if we have to
> + */
> + oldpath = make_pathname(settings_config_dir(), "bitzi.orig");
> + path = make_pathname(settings_config_dir(), "bitzi.xml");
I'd prefer "bitzi.xml.orig" for consistency with the other files.
Do you ever free ``oldpath'' and ``path''?
> + result = rename(path, oldpath);
> + if (result)
> + {
> + g_warning("bitzi_init: failed to rename %s to %s (%d)",
> + path, oldpath, result);
You want g_strerror(errno). ``result'' is quite useless.
> + }
> +
> + /*
> + * Set up the file cache descriptor, starting from scratch.
> + */
> + bitzi_cache_file = fopen(path, "w");
There's an error check somewhere, isn't there? Do you ever close it?
> + /*
> + * "play" the .orig file back through the XML parser and
> + * repopulate our internal cache
> + */
> + old_data = fopen(oldpath, "r");
> +
> + if (old_data)
> + {
> + bitzi_request_t *request=NULL;
> + char tmp[1024];
> +
> + while (fgets(tmp, sizeof(tmp), old_data))
> + {
Keep in mind that the line might be truncated now.
> + gint result;
> + int len;
Lengths of strings or memory chunks are (almost) always measured in size_t
not int.
> + if (strncmp(tmp,"<?xml",5)==0)
The "5" is, of course, correct but not nice. We probably need something
like is_prefix() to get rid of such strncmp()s.
> + {
> + if (request)
> + {
> + /* finish parsing */
> + result = xmlParseChunk(request->ctxt,
> tmp, 0, 1);
result?
> + /*
> + * the first line of bitzi.orig should always be the
> start
> + * of a ticket
> + */
> + g_assert(request!=NULL);
Well what if the file was damaged? Is that a reason to crash?
> + g_warning("Failed to open %s for cached Bitzi data (%s)",
> + oldpath, strerror(errno));
Use g_strerror() instead of strerror().
> g_timeout_add(1 * 10000, (GSourceFunc) bitzi_heartbeat, NULL);
Better get rid of the cast.
--
Christian
pgpefBb8ufoqO.pgp
Description: PGP signature
