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

[Jason Wang <jasowang@redhat.com>]

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.