[ccan] RFC: draft rfc822 module
David Gibson
david at gibson.dropbear.id.au
Tue May 22 23:50:59 EST 2012
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
More information about the ccan
mailing list