[ccan] RFC: draft rfc822 module

[David Gibson]

On Tue, May 22, 2012 at 01:22:58PM +0930, Paul 'Rusty' Russell wrote:
> 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/

Yeah, someone pointed that one out to me already.

> > +#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.

Ok.

> 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).

That makes sense.  Though it would be #if !HAVE_MEMMEM in this case.

> > +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.

Hrm, ok.

> 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);

Ok.

> > +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.

Heh, it's nothing that subtle.  I just copied the comment boilerplate
from container_of.h and forgot to update the actual content..

> > +/**
> > + * 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!

Hm, true.  ISTR I did that for a reason, which I have now forgotten.

> > +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.

Ok.

> > +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?

Yeah, fork() might be simpler.  And, um, apparently not.  At least not
when run in the configurations that ccanlint invokes.

> 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.

True, I should, hairy though that gets.

> > +/* 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.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson