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

[Zhang Chen <zhangchen.fnst@cn.fujitsu.com>]

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