Re: [Xen-devel] Re: [PATCH] vif-common.sh to support tap network devices in iptables FORWARD chain

[Teck Choon Giam <giamteckchoon@xxxxxxxxx>]

Hi,

On Tue, Jul 14, 2009 at 7:45 AM, Simon Horman<horms@xxxxxxxxxxxx> wrote:
> Hi,
>
> On Tue, Jul 07, 2009 at 07:49:15PM +0800, Teck Choon Giam wrote:
>> Sorry, the previous patch I sent in only support xm create to add in
>> iptables FORWARD chain but when you xm shutdown the tap related
>> ruleset is not removed from iptables FORWARD chain.  Below is the
>> patch which support xm create and xm shutdown.
>>
>> --- vif-common.sh.orig  2009-07-07 19:09:39.000000000 +0800
>> +++ vif-common.sh       2009-07-07 19:47:48.000000000 +0800
>> @@ -73,6 +73,24 @@
>>      local c="-D"
>>    fi
>>
>> +  # Added support for tap network devices in iptables FORWARD chain as this
>> +  # is required if antispoof is enabled or otherwise all packets to/from tap
>> +  # devices will be dropped.
>> +  # Start adding by Giam Teck Choon.
>
> Its not necessary to add comments that read like a changelog as
> they go in the changelog which is included in the version control system.
> Rather, comments in the code should just explain what the code does.

Then there isn't a need to have such comments in the patch I submit.
I will remove the comments then if the patch is fine.

>
>> +  local tapif=`echo $vif | sed 's/vif/tap/'`
>> +  # for xm create
>> +  local checktapif=`cat /proc/net/dev | grep "${tapif}:" | grep -v grep`
>
> Why is the second grep needed?

This is just my habit to include grep -v grep and you are free to
remove it.  Some shell scripts I coded needed that if the grep result
grep itself especially for ps fauwx related.

>> +  # for xm shutdown
>> +  local checktapstate=`iptables -L -n | grep "state
>> RELATED,ESTABLISHED PHYSDEV match --physdev-out ${tapif}"`
>> +
>> +  if [ -n "$checktapif" ] || [ -n "$checktapstate" ] ; then
>> +    iptables "$c" FORWARD -m physdev --physdev-in "$tapif" "$@" -j ACCEPT \
>> +      2>/dev/null &&
>> +    iptables "$c" FORWARD -m state --state RELATED,ESTABLISHED -m physdev \
>> +      --physdev-out "$tapif" -j ACCEPT 2>/dev/null
>> +  fi
>> +  # End adding by Giam Teck Choon.
>
> Comments like this are not necessary either.

Ok noted.

Thanks.

Kindest regards,
Giam Teck Choon

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel