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

[Jason Wang]

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.