Date: Fri, 27 Jul 2012 17:30:15 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH 03/13] sprinter: Add a string_len method
Message-ID: <20120727213015.GC8502@mit.edu>
References: <1343183693-17134-1-git-send-email-amdragon@mit.edu>
	<1343183693-17134-4-git-send-email-amdragon@mit.edu>
	<87a9ynis8s.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87a9ynis8s.fsf@qmul.ac.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Mark Walters on Jul 25 at  7:57 pm:
> 
> Hi
> 
> I have read this series (apart from the test changes) and it is both
> very nice to review and looks good. It builds and all tests pass at all
> the interesting partial stages and when fully applied. I have a few very
> minor comments.
> 
> On Wed, 25 Jul 2012, Austin Clements <amdragon@MIT.EDU> wrote:
> > This method allows callers to output strings with specific lengths.
> > It's useful both for strings with embedded NULs (which JSON can
> > represent, though parser support is apparently spotty), and
> > non-terminated strings.
> > ---
> >  sprinter-json.c |   11 +++++++++--
> >  sprinter-text.c |   11 +++++++++--
> >  sprinter.h      |    1 +
> >  3 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/sprinter-json.c b/sprinter-json.c
> > index 4649655..2587ca6 100644
> > --- a/sprinter-json.c
> > +++ b/sprinter-json.c
> > @@ -89,7 +89,7 @@ json_end (struct sprinter *sp)
> >  }
> >  
> >  static void
> > -json_string (struct sprinter *sp, const char *val)
> > +json_string_len (struct sprinter *sp, const char *val, size_t len)
> >  {
> 
> I think this function could do with a comment along the lines of the
> commit message. It might be nice to document somewhere where/when we

Will do.  I put a comment on this function explaining how it handles
NULs, as well as a comment on the generic vtable pointer explaining
string_len.

> might actually have nulls in an encoded string (are they allowed in
> bodies, headers etc).

Interestingly, RFC822, while being limited to 7-bit ASCII, explicitly
did allow NULs [RFC822, CHAR non-terminal].  RFC2822 explicitly
obsoletes them [RFC2822 4 or appendix B], which means consumers should
support them, but generators must not generate them.  As usual, MIME
makes things more complicated by supporting a "binary" content
transfer encoding that does permits NULs.  However, good luck getting
that through SMTP business; SMTP theoretically allows any ASCII
control character, but the standard specifically warns that control
characters other than SP, HT, CR, and LF should be avoided [RFC2821
4.1.1.4].  Even if you've negotiated the 8BITMIME extension [RFC1652],
you're limited by what MIME's "8bit" content transfer encoding
permits, which differs from the "binary" content transfer encoding by
disallowing (guess what) NULs and long lines.

In other words: yes.

> Actually, do we know that the json emacs parser handles nulls correctly?

It does.  Emacs strings are clean, so json.el doesn't have to do
special anything to support them.

> Best wishes
> 
> Mark
> >      static const char *const escapes[] = {
> >  	['\"'] = "\\\"", ['\\'] = "\\\\", ['\b'] = "\\b",
> > @@ -98,7 +98,7 @@ json_string (struct sprinter *sp, const char *val)
> >      struct sprinter_json *spj = json_begin_value (sp);
> >  
> >      fputc ('"', spj->stream);
> > -    for (; *val; ++val) {
> > +    for (; len; ++val, --len) {
> >  	unsigned char ch = *val;
> >  	if (ch < ARRAY_SIZE (escapes) && escapes[ch])
> >  	    fputs (escapes[ch], spj->stream);
> > @@ -111,6 +111,12 @@ json_string (struct sprinter *sp, const char *val)
> >  }
> >  
> >  static void
> > +json_string (struct sprinter *sp, const char *val)
> > +{
> > +    json_string_len (sp, val, strlen (val));
> > +}
> > +
> > +static void
> >  json_integer (struct sprinter *sp, int val)
> >  {
> >      struct sprinter_json *spj = json_begin_value (sp);
> > @@ -166,6 +172,7 @@ sprinter_json_create (const void *ctx, FILE *stream)
> >  	    .begin_list = json_begin_list,
> >  	    .end = json_end,
> >  	    .string = json_string,
> > +	    .string_len = json_string_len,
> >  	    .integer = json_integer,
> >  	    .boolean = json_boolean,
> >  	    .null = json_null,
> > diff --git a/sprinter-text.c b/sprinter-text.c
> > index b208840..dfa54b5 100644
> > --- a/sprinter-text.c
> > +++ b/sprinter-text.c
> > @@ -25,14 +25,20 @@ struct sprinter_text {
> >  };
> >  
> >  static void
> > -text_string (struct sprinter *sp, const char *val)
> > +text_string_len (struct sprinter *sp, const char *val, size_t len)
> >  {
> >      struct sprinter_text *sptxt = (struct sprinter_text *) sp;
> >  
> >      if (sptxt->current_prefix != NULL)
> >  	fprintf (sptxt->stream, "%s:", sptxt->current_prefix);
> >  
> > -    fputs(val, sptxt->stream);
> > +    fwrite (val, len, 1, sptxt->stream);
> > +}
> > +
> > +static void
> > +text_string (struct sprinter *sp, const char *val)
> > +{
> > +    text_string_len (sp, val, strlen (val));
> >  }
> >  
> >  static void
> > @@ -105,6 +111,7 @@ sprinter_text_create (const void *ctx, FILE *stream)
> >  	    .begin_list = text_begin_list,
> >  	    .end = text_end,
> >  	    .string = text_string,
> > +	    .string_len = text_string_len,
> >  	    .integer = text_integer,
> >  	    .boolean = text_boolean,
> >  	    .null = text_null,
> > diff --git a/sprinter.h b/sprinter.h
> > index 6680d41..826a852 100644
> > --- a/sprinter.h
> > +++ b/sprinter.h
> > @@ -28,6 +28,7 @@ typedef struct sprinter {
> >       * For string, the char * must be UTF-8 encoded.
> >       */
> >      void (*string) (struct sprinter *, const char *);
> > +    void (*string_len) (struct sprinter *, const char *, size_t);
> >      void (*integer) (struct sprinter *, int);
> >      void (*boolean) (struct sprinter *, notmuch_bool_t);
> >      void (*null) (struct sprinter *);
> >
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch
> 

-- 
Austin Clements                                      MIT/'06/PhD/CSAIL
amdragon@mit.edu                           http://web.mit.edu/amdragon
       Somewhere in the dream we call reality you will find me,
              searching for the reality we call dreams.