From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:55666)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jasowang@redhat.com>) id 1bNAJi-0005H3-Te
	for qemu-devel@nongnu.org; Tue, 12 Jul 2016 22:54:31 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <jasowang@redhat.com>) id 1bNAJe-0007FH-Pr
	for qemu-devel@nongnu.org; Tue, 12 Jul 2016 22:54:29 -0400
Received: from mx1.redhat.com ([209.132.183.28]:39005)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jasowang@redhat.com>) id 1bNAJe-0007FC-HB
	for qemu-devel@nongnu.org; Tue, 12 Jul 2016 22:54:26 -0400
References: <1466681677-30487-1-git-send-email-zhangchen.fnst@cn.fujitsu.com>
	<1466681677-30487-5-git-send-email-zhangchen.fnst@cn.fujitsu.com>
	<577F6B8D.2050309@redhat.com> <57836EAB.5090007@cn.fujitsu.com>
From: Jason Wang <jasowang@redhat.com>
Message-ID: <5785AD5C.6040903@redhat.com>
Date: Wed, 13 Jul 2016 10:54:20 +0800
MIME-Version: 1.0
In-Reply-To: <57836EAB.5090007@cn.fujitsu.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [RFC PATCH V5 4/4] colo-compare: add TCP, UDP,
 ICMP packet comparison
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>, qemu devel <qemu-devel@nongnu.org>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>, "eddie . dong" <eddie.dong@intel.com>, "Dr . David Alan Gilbert" <dgilbert@redhat.com>, zhanghailiang <zhang.zhanghailiang@huawei.com>



On 2016=E5=B9=B407=E6=9C=8811=E6=97=A5 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 =3D ppkt->ip->ip_hl * 4;
>>> +    if (ppkt->size !=3D spkt->size ||
>>> +        ppkt->size < network_length + ETH_HLEN) {
>>> + trace_colo_compare_icmp_miscompare_size(ppkt->size, spkt->size);
>>> +        return -1;
>>> +    }
>>> +    icmp_ppkt =3D (struct icmp *)(ppkt->data + network_length +=20
>>> ETH_HLEN);
>>> +    icmp_spkt =3D (struct icmp *)(spkt->data + network_length +=20
>>> ETH_HLEN);
>>> +
>>> +    if ((icmp_ppkt->icmp_type =3D=3D icmp_spkt->icmp_type) &&
>>> +        (icmp_ppkt->icmp_code =3D=3D icmp_spkt->icmp_code)) {
>>> +        if (icmp_ppkt->icmp_type =3D=3D ICMP_REDIRECT) {
>>> +            if (icmp_ppkt->icmp_gwaddr.s_addr !=3D
>>> +                icmp_spkt->icmp_gwaddr.s_addr) {
>>> +                trace_colo_compare_main("icmp_gwaddr.s_addr not=20
>>> 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 =3D=3D ICMP_UNREACH) &&
>>> +                   (icmp_ppkt->icmp_type =3D=3D ICMP_UNREACH_NEEDFRA=
G)) {
>>> +            if (icmp_ppkt->icmp_nextmtu !=3D 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=20

I believe we should cover all instead of "most" of situations. And looks=20
like icmp packet were all small, so there's probably no need to do=20
special tricks like this.