Re: [Qemu-devel] [RFC PATCH V5 4/4] colo-compare: add TCP, UDP, ICMP packet comparison

[Zhang Chen]

On 07/13/2016 10:54 AM, Jason Wang wrote:
> On 2016年07月11日 18:02, Zhang Chen wrote:
> > > > +static int colo_packet_compare_icmp(Packet *spkt, Packet *ppkt)
> > > > +{
> > > > +    int network_length;
> > > > +    struct icmp *icmp_ppkt, *icmp_spkt;
> > > > +
> > > > +    trace_colo_compare_main("compare icmp");
> > > > +    network_length = ppkt->ip->ip_hl * 4;
> > > > +    if (ppkt->size != spkt->size ||
> > > > +        ppkt->size < network_length + ETH_HLEN) {
> > > > + trace_colo_compare_icmp_miscompare_size(ppkt->size, spkt->size);
> > > > +        return -1;
> > > > +    }
> > > > +    icmp_ppkt = (struct icmp *)(ppkt->data + network_length + 
> > > > ETH_HLEN);
> > > > +    icmp_spkt = (struct icmp *)(spkt->data + network_length + 
> > > > ETH_HLEN);
> > > > +
> > > > +    if ((icmp_ppkt->icmp_type == icmp_spkt->icmp_type) &&
> > > > +        (icmp_ppkt->icmp_code == icmp_spkt->icmp_code)) {
> > > > +        if (icmp_ppkt->icmp_type == ICMP_REDIRECT) {
> > > > +            if (icmp_ppkt->icmp_gwaddr.s_addr !=
> > > > +                icmp_spkt->icmp_gwaddr.s_addr) {
> > > > +                trace_colo_compare_main("icmp_gwaddr.s_addr not 
> > > > same");
> > > > + trace_colo_compare_icmp_miscompare_addr("ppkt s_addr",
> > > > + inet_ntoa(icmp_ppkt->icmp_gwaddr));
> > > > + trace_colo_compare_icmp_miscompare_addr("spkt s_addr",
> > > > + inet_ntoa(icmp_spkt->icmp_gwaddr));
> > > > +                return -1;
> > > > +            }
> > > > +        } else if ((icmp_ppkt->icmp_type == ICMP_UNREACH) &&
> > > > +                   (icmp_ppkt->icmp_type == ICMP_UNREACH_NEEDFRAG)) {
> > > > +            if (icmp_ppkt->icmp_nextmtu != icmp_spkt->icmp_nextmtu) {
> > > > +                trace_colo_compare_main("icmp_nextmtu not same");
> > > > + trace_colo_compare_icmp_miscompare_mtu("ppkt nextmtu",
> > > > + icmp_ppkt->icmp_nextmtu);
> > > > + trace_colo_compare_icmp_miscompare_mtu("spkt nextmtu",
> > > > + icmp_spkt->icmp_nextmtu);
> > > > +                return -1;
> > > > +            }
> > > > +        }
> > > > +    } else {
> > > > +        return -1;
> > > > +    }
> > >
> > > Why only compare part of icmp packet?
> >
> > That's include most of situation, increase all part of icmp
> > can reduce compare efficiency.
> >
> > Thanks
> > Zhang Chen 
>
> I believe we should cover all instead of "most" of situations. And 
> looks like icmp packet were all small, so there's probably no need to 
> do special tricks like this.

OK, I will fix this in next version.

Thanks
Zhang Chen

> .

--
Thanks
zhangchen