From 85bf6595ec8dcff3eb8c6843cc6c0b574d3e5423 Mon Sep 17 00:00:00 2001
From: Teddy_Zhu <teddyzhu15@gmail.com>
Date: Tue, 16 Apr 2024 19:17:15 +0800
Subject: [PATCH] fix: fix qos warn && qos rules redundancy when ip changed
 (#59)

---
 pkg/network/iptables.go |  7 +++---
 pkg/switch/qos.go       | 56 ++++++++++++++++++++++++-----------------
 2 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/pkg/network/iptables.go b/pkg/network/iptables.go
index 9aa646e..3b0957f 100755
--- a/pkg/network/iptables.go
+++ b/pkg/network/iptables.go
@@ -129,16 +129,15 @@ func (ru IPRule) Args() []string {
 	if ru.Output != "" {
 		args = append(args, "-o", ru.Output)
 	}
-	if ru.Comment != "" {
-		args = append(args, "-m", "comment", "--comment", ru.Comment)
-	}
-
 	if ru.Limit != "" {
 		args = append(args, "-m", "limit", "--limit", ru.Limit)
 	}
 	if ru.LimitBurst != "" {
 		args = append(args, "--limit-burst", ru.LimitBurst)
 	}
+	if ru.Comment != "" {
+		args = append(args, "-m", "comment", "--comment", ru.Comment)
+	}
 
 	if ru.Jump != "" {
 		jump := strings.ToUpper(ru.Jump)
diff --git a/pkg/switch/qos.go b/pkg/switch/qos.go
index 178b9bf..6aa81e3 100644
--- a/pkg/switch/qos.go
+++ b/pkg/switch/qos.go
@@ -62,7 +62,7 @@ func (qr *QosUser) BuildChainIn(chain *cn.FireWallChain) {
 }
 
 func (qr *QosUser) BuildChainInJump(chain *cn.FireWallChain) {
-	if qr.Ip != "" {
+	if qr.Ip != "" && qr.InSpeed > 0 {
 		if err := chain.AddRuleX(cn.IPRule{
 			Comment: "Qos Jump",
 			Jump:    qr.RuleName("in"),
@@ -74,12 +74,15 @@ func (qr *QosUser) BuildChainInJump(chain *cn.FireWallChain) {
 }
 
 func (qr *QosUser) ClearChainInJump(chain *cn.FireWallChain) {
-	if err := chain.DelRuleX(cn.IPRule{
-		Comment: "Qos Jump",
-		Jump:    qr.RuleName("in"),
-		Source:  qr.Ip,
-	}); err != nil {
-		qr.out.Warn("Qos.Del In Rule: %s", err)
+	if qr.Ip != "" && qr.InSpeed > 0 {
+		qr.out.Debug("ClearChainInJump: %s", qr.Ip)
+		if err := chain.DelRuleX(cn.IPRule{
+			Comment: "Qos Jump",
+			Jump:    qr.RuleName("in"),
+			Source:  qr.Ip,
+		}); err != nil {
+			qr.out.Warn("Qos.Del In Rule: %s", err)
+		}
 	}
 }
 
@@ -94,8 +97,12 @@ func (qr *QosUser) ReBuild(chainIn *cn.FireWallChain) {
 
 func (qr *QosUser) ClearChainIn(chain *cn.FireWallChain) {
 	if qr.qosChainIn != nil {
-		qr.ClearChainInJump(chain)
+		qr.out.Debug("qos chain ClearChainIn start")
+		if qr.Ip != "" {
+			qr.ClearChainInJump(chain)
+		}
 		qr.qosChainIn.Cancel()
+
 		qr.qosChainIn = nil
 	}
 }
@@ -105,23 +112,26 @@ func (qr *QosUser) Clear(chainIn *cn.FireWallChain) {
 }
 
 func (qr *QosUser) Update(chainIn *cn.FireWallChain, inSpeed float64, device string, ip string) {
-
-	changed := false
 	qr.Device = device
-	if qr.Ip != ip {
-		changed = true
+
+	ipChanged := qr.Ip != ip
+	speedChanged := qr.InSpeed != inSpeed
+
+	if speedChanged {
+		// speed will rebuild jump & limit
+		qr.ClearChainIn(chainIn)
+		qr.InSpeed = inSpeed
 		qr.Ip = ip
+		qr.BuildChainIn(chainIn)
+		return
 	}
 
-	if changed {
+	if ipChanged {
 		qr.ClearChainInJump(chainIn)
+		qr.Ip = ip
 		qr.BuildChainInJump(chainIn)
-	}
-
-	if qr.InSpeed != inSpeed {
-		qr.InSpeed = inSpeed
-		qr.ClearChainIn(chainIn)
-		qr.BuildChainIn(chainIn)
+	} else {
+		//ignored
 	}
 
 }
@@ -250,6 +260,8 @@ func (q *QosCtrl) AddOrUpdateQosUser(name string, inSpeed float64) {
 }
 
 func (q *QosCtrl) ClientUpdate() {
+	q.lock.Lock()
+	defer q.lock.Unlock()
 	clients := make([]schema.VPNClient, 0, 1024)
 	for n := range cache.Network.List() {
 		if n == nil {
@@ -273,10 +285,8 @@ func (q *QosCtrl) ClientUpdate() {
 		if existClient != nil {
 			rule.Update(q.chainIn, rule.InSpeed, existClient.Device, existClient.Address)
 		} else {
-			if rule.Ip != "" {
-				rule.ClearChainInJump(q.chainIn)
-				rule.Ip = ""
-			}
+			rule.ClearChainInJump(q.chainIn)
+			rule.Ip = ""
 		}
 	}