Skip to content

Refactor to reuse dig command and improve maintainability #37

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor to reuse dig command and improve maintainability #37

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions chkdm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ for cmd in dig nslookup sed head awk sort dirname readlink; do
fi
done

dig_command=(dig +short)

shopt -s extglob
domain="${1%%+(.)}"
# RFC 1035, 1123, 2181, 5891 compliant domain validation
Expand Down Expand Up @@ -99,14 +101,14 @@ adblockDNS[Tiarap]="188.166.206.224"

PaloAltoSinkholeCname="sinkhole.paloaltonetworks.com."
NextDNSBlockPageCname="blockpage.nextdns.io."
NextDNSBlockPageIP="$(dig +short $NextDNSBlockPageCname)"
NextDNSBlockPageIP="$("${dig_command[@]}" $NextDNSBlockPageCname)"

function query() {
domain="$1"
dns="$2"
filterDetect="$3"

result="$(dig +short "$domain" @"$dns" | xargs)"
result="$("${dig_command[@]}" "$domain" @"$dns" | xargs)"
echo "$result"

if echo "$result" | grep -Eq "(^| )$PaloAltoSinkholeCname($| )"; then
Expand Down Expand Up @@ -186,7 +188,7 @@ function chkDomain() {
}

function warnUpDNS() {
echo "${nofilterDNS[*]} ${secureDNS[*]} ${adblockDNS[*]}" | sed 's/ /\n/g' | xargs -I{} -P5 dig +short "$domain" @{} > /dev/null 2>&1 &
echo "${nofilterDNS[*]} ${secureDNS[*]} ${adblockDNS[*]}" | sed 's/ /\n/g' | xargs -I{} -P5 "${dig_command[@]}" "$domain" @{} > /dev/null 2>&1 &
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve array handling in parallel execution.

While the array expansion is correct, the current implementation might have issues if the dig command contains spaces or special characters when used with xargs.

Consider restructuring the command to handle the array expansion more safely:

-    echo "${nofilterDNS[*]} ${secureDNS[*]} ${adblockDNS[*]}" | sed 's/ /\n/g' | xargs -I{} -P5 "${dig_command[@]}" "$domain" @{} > /dev/null 2>&1 &
+    echo "${nofilterDNS[*]} ${secureDNS[*]} ${adblockDNS[*]}" | sed 's/ /\n/g' | while read -r dns; do
+        "${dig_command[@]}" "$domain" "@$dns" > /dev/null 2>&1 &
+        # Limit concurrent processes
+        while [ "$(jobs -r | wc -l)" -ge 5 ]; do
+            sleep 0.1
+        done
+    done

This approach:

  1. Safely handles array expansion
  2. Maintains parallel execution limit
  3. Properly handles DNS servers with special characters
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
echo "${nofilterDNS[*]} ${secureDNS[*]} ${adblockDNS[*]}" | sed 's/ /\n/g' | xargs -I{} -P5 "${dig_command[@]}" "$domain" @{} > /dev/null 2>&1 &
echo "${nofilterDNS[*]} ${secureDNS[*]} ${adblockDNS[*]}" | sed 's/ /\n/g' | while read -r dns; do
"${dig_command[@]}" "$domain" "@$dns" > /dev/null 2>&1 &
# Limit concurrent processes
while [ "$(jobs -r | wc -l)" -ge 5 ]; do
sleep 0.1
done
done

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there will be special characters, and currently, it does contain a space, but it also works as expected. Is there something wrong?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeterDaveHello You're right. I apologize for the overly cautious suggestion. The current implementation is working correctly because:

  1. The array expansion "${dig_command[@]}" properly handles the space in dig +short
  2. The xargs usage with -I{} safely handles the DNS server parameter
  3. The parallel execution with -P5 works as intended

The suggested change would add unnecessary complexity without providing tangible benefits. Let's keep the current implementation.

✏️ Learnings added 
Learnt from: PeterDaveHello
PR: PeterDaveHello/chkdomain#37
File: chkdm:191-191
Timestamp: 2025-01-14T16:38:18.403Z
Learning: The parallel DNS query implementation in chkdomain using `xargs -I{} -P5` with array expansion `"${dig_command[@]}"` works correctly for handling spaces in the dig command and DNS server parameters. The suggestion to use a while loop with jobs control would add unnecessary complexity.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

}

function checkNofilterDNS() {
Expand Down