Wireshark dissector for SPDY protocol.

Wireshark dissector for SPDY protocol.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=39730

[cbentzel, 2010-02-22 21:23:10]

This is just a straight add of the existing files. I did not add build
instructions yet.

[Eric Shienbrood, 2010-02-22 21:36:54]

LGTM

[Mike Belshe, 2010-02-22 21:51:19]

Thanks, Chris.  LGTM - I am still reviewing the .c and .h, but we can do those
separately too - or maybe you have already reviewed them.

http://codereview.chromium.org/650167/diff/1/3
File net/tools/spdyshark/COPYING (right):

http://codereview.chromium.org/650167/diff/1/3#newcode334
net/tools/spdyshark/COPYING:334: Ty Coon, President of Vice
This looks fishy :-)

Do we need this file?

[Mike Belshe, 2010-02-22 21:55:07]

Here are a few more comments.  Maybe we should land first, then address the
style nits.

http://codereview.chromium.org/650167/diff/1/12
File net/tools/spdyshark/packet-spdy.c (right):

http://codereview.chromium.org/650167/diff/1/12#newcode179
net/tools/spdyshark/packet-spdy.c:179: #define TCP_PORT_DAAP			3689
Remove?

http://codereview.chromium.org/650167/diff/1/12#newcode184
net/tools/spdyshark/packet-spdy.c:184: #define TCP_PORT_SSDP			1900
Remove?

http://codereview.chromium.org/650167/diff/1/12#newcode278
net/tools/spdyshark/packet-spdy.c:278: if (spdy_debug) printf("Should reset SPDY
decompressors\n");
nit: 2 lines

http://codereview.chromium.org/650167/diff/1/12#newcode284
net/tools/spdyshark/packet-spdy.c:284: conversation_t  *conversation;
nit: conversation_t* conversation.
  there are more like these.

http://codereview.chromium.org/650167/diff/1/12#newcode288
net/tools/spdyshark/packet-spdy.c:288: conversation =
find_conversation(pinfo->fd->num, &pinfo->src, &pinfo->dst, pinfo->ptype,
pinfo->srcport, pinfo->destport, 0);
nit: 80 cols.

http://codereview.chromium.org/650167/diff/1/12#newcode297
net/tools/spdyshark/packet-spdy.c:297: conversation =
conversation_new(pinfo->fd->num, &pinfo->src, &pinfo->dst, pinfo->ptype,
pinfo->srcport, pinfo->destport, 0);
nit: 80cols

http://codereview.chromium.org/650167/diff/1/12#newcode773
net/tools/spdyshark/packet-spdy.c:773: sizeof(spdy_dictionary));
This should be sizeof(spdy_dictionary)-1.  Probably why things are failing. 
Maybe make it a constant?

[cbentzel, 2010-02-22 22:04:50]

I was thinking of just landing first, and then addressing issues.

On Mon, Feb 22, 2010 at 4:55 PM, <mbelshe@chromium.org> wrote:

> Here are a few more comments.  Maybe we should land first, then address the
> style nits.
>
>
> http://codereview.chromium.org/650167/diff/1/12
> File net/tools/spdyshark/packet-spdy.c (right):
>
> http://codereview.chromium.org/650167/diff/1/12#newcode179
> net/tools/spdyshark/packet-spdy.c:179: #define TCP_PORT_DAAP
>      3689
> Remove?
>
> http://codereview.chromium.org/650167/diff/1/12#newcode184
> net/tools/spdyshark/packet-spdy.c:184: #define TCP_PORT_SSDP
>      1900
> Remove?
>
> http://codereview.chromium.org/650167/diff/1/12#newcode278
> net/tools/spdyshark/packet-spdy.c:278: if (spdy_debug) printf("Should
> reset SPDY decompressors\n");
> nit: 2 lines
>
> http://codereview.chromium.org/650167/diff/1/12#newcode284
> net/tools/spdyshark/packet-spdy.c:284: conversation_t  *conversation;
> nit: conversation_t* conversation.
>  there are more like these.
>
> http://codereview.chromium.org/650167/diff/1/12#newcode288
> net/tools/spdyshark/packet-spdy.c:288: conversation =
> find_conversation(pinfo->fd->num, &pinfo->src, &pinfo->dst,
> pinfo->ptype, pinfo->srcport, pinfo->destport, 0);
> nit: 80 cols.
>
> http://codereview.chromium.org/650167/diff/1/12#newcode297
> net/tools/spdyshark/packet-spdy.c:297: conversation =
> conversation_new(pinfo->fd->num, &pinfo->src, &pinfo->dst, pinfo->ptype,
> pinfo->srcport, pinfo->destport, 0);
> nit: 80cols
>
> http://codereview.chromium.org/650167/diff/1/12#newcode773
> net/tools/spdyshark/packet-spdy.c:773: sizeof(spdy_dictionary));
> This should be sizeof(spdy_dictionary)-1.  Probably why things are
> failing.  Maybe make it a constant?
>
>
> http://codereview.chromium.org/650167
>

[Eric Shienbrood, 2010-02-22 23:30:32]

http://codereview.chromium.org/650167/diff/1/12
File net/tools/spdyshark/packet-spdy.c (right):

http://codereview.chromium.org/650167/diff/1/12#newcode278
net/tools/spdyshark/packet-spdy.c:278: if (spdy_debug) printf("Should reset SPDY
decompressors\n");
On 2010/02/22 21:55:08, Mike Belshe wrote:
> nit: 2 lines

C++ style guide says:
"Short conditional statements may be written on one line if this enhances
readability."

http://codereview.chromium.org/650167/diff/1/12#newcode284
net/tools/spdyshark/packet-spdy.c:284: conversation_t  *conversation;
On 2010/02/22 21:55:08, Mike Belshe wrote:
> nit: conversation_t* conversation.
>   there are more like these. 

I'm not sure we should be following the Google style guide here. I tried to
conform to the style used in the rest of the wireshark sources. The style guide
says:
"When modifying code written outside Google, follow the conventions within that
code for consistency."

http://codereview.chromium.org/650167/diff/1/12#newcode773
net/tools/spdyshark/packet-spdy.c:773: sizeof(spdy_dictionary));
On 2010/02/22 21:55:08, Mike Belshe wrote:
> This should be sizeof(spdy_dictionary)-1.  Probably why things are failing. 
> Maybe make it a constant?

Has this changed? It should have been sizeof(...)-1 all along, but
flip_framer.cc was using sizeof(...), so I had to mimic that here for it to
work.

[cbentzel, 2010-02-23 02:21:58]

I think I'll just commit this tomorrow morning and then start addressing the
issues.

http://codereview.chromium.org/650167/diff/1/12
File net/tools/spdyshark/packet-spdy.c (right):

http://codereview.chromium.org/650167/diff/1/12#newcode284
net/tools/spdyshark/packet-spdy.c:284: conversation_t  *conversation;
On 2010/02/22 23:30:32, Eric Shienbrood wrote:
> On 2010/02/22 21:55:08, Mike Belshe wrote:
> > nit: conversation_t* conversation.
> >   there are more like these. 
> 
> I'm not sure we should be following the Google style guide here. I tried to
> conform to the style used in the rest of the wireshark sources. The style
guide
> says:
> "When modifying code written outside Google, follow the conventions within
that
> code for consistency."

Agree. I think the ultimate goal is to get this integrated with wireshark
mainline so it should follow those code conventions rather than Chrome's.

[cbentzel, 2010-02-23 18:04:50]

I landed this earlier today. I'll try to find some time to address the issues
you already raised.

On 2010/02/23 02:21:58, cbentzel wrote:
> I think I'll just commit this tomorrow morning and then start addressing the
> issues.
> 
> http://codereview.chromium.org/650167/diff/1/12
> File net/tools/spdyshark/packet-spdy.c (right):
> 
> http://codereview.chromium.org/650167/diff/1/12#newcode284
> net/tools/spdyshark/packet-spdy.c:284: conversation_t  *conversation;
> On 2010/02/22 23:30:32, Eric Shienbrood wrote:
> > On 2010/02/22 21:55:08, Mike Belshe wrote:
> > > nit: conversation_t* conversation.
> > >   there are more like these. 
> > 
> > I'm not sure we should be following the Google style guide here. I tried to
> > conform to the style used in the rest of the wireshark sources. The style
> guide
> > says:
> > "When modifying code written outside Google, follow the conventions within
> that
> > code for consistency."
> 
> Agree. I think the ultimate goal is to get this integrated with wireshark
> mainline so it should follow those code conventions rather than Chrome's.