[ccan] RFC: draft rfc822 module

[Rusty Russell]

On Mon, 21 May 2012 11:12:37 +1000, David Gibson <david at gibson.dropbear.id.au> wrote:
> Here's a draft of a module for parsing RFC822(/RFC2822/RFC5322)
> messages.  It doesn't do a heap at the moment, and it needs some work
> on stuff to match the RFC better, but here's what I have now for
> comment.

Comments below.  First up: this is really looking quite neat!

> + * - Be lazy.  Don't compute immediately compute fancy indexes for the
> + *   mesage.  Just reading messages into the system, then sending

s/mesage/message/

> +#ifndef HAVE_MEMMEM
> +static const void *memmem(const void *haystack, size_t haystacklen,
> +			  const void *needle, size_t needlelen)
> +{
> +	const char *p, *last;
> +
> +	p = haystack;
> +	last = p + haystacklen - needlelen;
> +
> +	do {
> +		if (memcmp(p, needle, needlelen) == 0)
> +			return p;
> +	} while (p++ <= last);
> +
> +	return NULL;
> +}
> +#endif

We should put this in str, I think.

Also, I prefer #if HAVE_MEMMEM.  It works the same, but then you can use
-Wundef and actually define it to 0 in config.h: you then get told if
you don't define it.  Catches misspellings, or new tests for which
there's nothing in config.h (yet).

> +void rfc822_check(struct rfc822_msg *msg)
> +{
> +	assert(msg);
> +	list_check(&msg->headers, "bad header list");
> +}

I would probably copy the list_check() method, where a
NULL abort string means we don't abort.

struct rfc822_msg *rfc822_check(const struct rfc822_msg *msg,
                                const char *abortstr)
{
        assert(msg);
        if (!list_check(&msg->headers, abortstr))
                return NULL;
        return (struct rfc822_msg *)msg;
}

This makes life easier for the callers, who can further wrap
rfc822_check() if they want to, and can insert it easily in their
code.

> +#ifdef CCAN_RFC822_DEBUG
> +#define CHECK(msg)	rfc822_check((msg))
> +#else
> +#define CHECK(msg)	do { } while (0)
> +#endif

Might want to put this in rfc822.h, as an extra usage hint:

/* #define CCAN_RFC822_DEBUG 1 */

And then do this:

#ifdef CCAN_RFC822_DEBUG
#define CCAN_LIST_DEBUG
#endif

Might also want to "#define CHECK(msg) (msg)" and use it like that, eg
      return CHECK(msg);

> +void rfc822_free(struct rfc822_msg *msg)
> +{
> +	talloc_free(msg);
> +}

Your comment says:
> +/**
> + * rfc822_free - get pointer to enclosing structure
> + * @msg:
> + *
> + * Given a pointer to a member of a structure, this macro does pointer
> + * subtraction to return the pointer to the enclosing type.
> + */
> +void rfc822_free(struct rfc822_msg *msg);

...which mismatches.  If you do such tricks, then msg can no longer be
treated by the caller as a talloc object, which is a bit nasty.  If you
don't intend to, you can get rid of rfc822_free.

> +/**
> + * rfc822_start - start parsing a new rfc822 message
> + * @ctx: talloc context to make allocations in
> + * @p: pointer to a buffer containing the message text
> + * @len: length of the message text
> + *
> + * This function creates a new rfc822_msg context for parsing an
> + * rfc822 message, initialized based on the message text given by the
> + * pointer.
> + */
> +struct rfc822_msg *rfc822_start(const void *ctx, const char *p, size_t len);

Note: will return NULL on allocation failure, not call the allocation
failure handler!

> +const struct rfc822_header *rfc822_next_header(struct rfc822_msg *msg,
> +					       const struct rfc822_header *hdr);
> +#define rfc822_first_header(msg) (rfc822_next_header((msg), NULL))

Put rfc822_first_header before rfc822_next_header.  That will make your
documentation nicer, and the compiler doesn't care.

> +static void abort_handler(int signum)
> +{
> +	ok1(1);
> +	exit(0);
> +}

Interesting.  I would have used fork() myself.  Doesn't valgrind report
a leak for this?

I've fixed that in the past by having my fake malloc record the
allocations and frees, and fixing it up before exiting.  Or suppress
that check for this test in _info.

> diff --git a/ccan/rfc822/test/run-bad-header.c b/ccan/rfc822/test/run-bad-header.c
> new file mode 100644
> index 0000000..1a7f01b
> --- /dev/null
> +++ b/ccan/rfc822/test/run-bad-header.c
> @@ -0,0 +1,82 @@
> +#include <ccan/foreach/foreach.h>
> +#include <ccan/failtest/failtest_override.h>
> +#include <ccan/failtest/failtest.h>
> +#include <ccan/tap/tap.h>
> +#include <stdlib.h>
> +#include <string.h>

Cool, failtest!

> +int main(int argc, char *argv[])
> +{
> +	failtest_init(argc, argv);
> +	rfc822_set_allocation_failure_handler(failtest_abort);
> +	talloc_set_allocator(my_malloc, my_free, my_realloc);
> +
> +	test_bad_header(bad_header, sizeof(bad_header));
> +
> +	/* This exits depending on whether all tests passed */
> +	failtest_exit(exit_status());

This is a very nice test.  You might want to test with a failure
handler which doesn't exit, though (since you allow that).  Should
be a very simple matter of repeating this with an empty handler.

> +/* We want to test talloc failure paths. */
> +static void *my_malloc(size_t size)
> +{
> +	return malloc(size);
> +}

On my TODO is allowing the caller to extend failtest to their own
functions.  But I guess this works for the moment.

Nice work!

Cheers,
Rusty.