[PATCH 1 of 7] Mail: add IMAP ID command support

[Maxim Dounin]

Hello!

On Fri, Jan 17, 2014 at 10:57:39AM +0100, Filipe Da Silva wrote:

> Hi !
> 
> Many thanks for the reviews.
> I will rework on my patch quickly.
> 
> I've just one question.
> 
> 2014/1/17  <nginx-devel-request at nginx.org>:
> > Message: 1
> > Date: Fri, 17 Jan 2014 07:54:27 +0400
> > From: Maxim Dounin <mdounin at mdounin.ru>
> > To: nginx-devel at nginx.org
> > Subject: Re: [PATCH 1 of 7] Mail: add IMAP ID command support
> > Message-ID: <20140117035427.GM1835 at mdounin.ru>
> > Content-Type: text/plain; charset=us-ascii
> >
> > Hello!
> >
> > On Tue, Jan 14, 2014 at 12:54:18PM +0100, Filipe da Silva wrote:
> >
> >> # HG changeset patch
> 
> >
> > Please use
> >
> > [diff]
> > showfunc=1
> >
> > in your hgrc.  Having function names in diffs greatly simplifies
> > reading patches.
> >
> 
> As I read it, i remember seeing it in some previous threads.
> You better add this point to http://nginx.org/en/docs/contributing_changes.html

Sure, we'll consider it.  On the other hand, we try to keep this 
page short to simplify reading, and this is already implicitly 
suggested by the patch provided as an example.

> >> +
> >> +        s->imap_client_id = ngx_mail_imap_client_id_nil;
> >> +
> >> +    } else {
> >> +        size = sizeof("ID (") - 1;
> >> +        for (i = 0; i < s->args.nelts; i++) {
> >> +            size += 1 + arg[i].len + 2; // 1 space plus 2 quotes
> >> +        }
> >> +
> >> +        data = ngx_pnalloc(c->pool, size);
> >> +        if (data == NULL) {
> >> +            return NGX_ERROR;
> >> +        }
> >> +
> >> +        p = ngx_cpymem(data, "ID (", sizeof("ID (") - 1);
> >> +        for (i = 0; i < s->args.nelts; i++) {
> >> +            *p++ = '"';
> >> +            p = ngx_cpymem(p, arg[i].data, arg[i].len);
> >> +            *p++ = '"';
> >> +            *p++ = ' ';
> >> +        }
> >> +        *--p = ')'; // replace last space
> >> +
> >> +        s->imap_client_id.len = size;
> >> +        s->imap_client_id.data = data;
> >
> > This doesn't look correct.  E.g., arguments may contain quotes.
> >
> 
> I think I better add
> @@ -249,6 +249,8 @@ typedef struct {
>      u_char                 *cmd_start;
>      u_char                 *arg_start;
>      u_char                 *arg_end;
> +    u_char                 *list_start;
> +    u_char                 *list_end;
>      ngx_uint_t              literal_len;
>  } ngx_mail_session_t;
> 
> And use this point pointers to extract the whole parameter list at once .
> 
> But I'm worried about the fact that a very long client command could
> be split/sparse in separated buffers ?
> Am I wrong ? ' cause I haven't dig enough into nginx internals, to
> answer to this point.

There is only one buffer, so using whole list should be fine.  On 
the other hand, there is no need to add additional pointers - the 
s->args should be enough, see e.g. how ngx_mail_smtp_rcpt() does 
it.

-- 
Maxim Dounin
http://nginx.org/