CMD closing instead of looping

Question

Im working on a simple script to log some time and ping info when a failure occurs, however when the ping is in place the script just closes.

:: setting variables/conditions for the loop
:: not interval is number of seconds between loop run

    set folder1="C:\Users\Joseph\Desktop\Testing_1"
    set num=0
    Set Interval=60 
    set IPAddress=8.8.8.8

:LoopStart
    
    ping -n 4 %IPADDRESS% | find "TTL=" > nul
        if errorlevel 1 set /a num=%num% + 1
        if errorlevel 1 set file1="%folder1%\25090_log_%num%.txt"
        if errorlevel 1 echo %date%-%time% >> %file1%
        if errorlevel 1 ping -n 1 %ipaddress% >>%file1%
    timeout %INTERVAL%
Goto LoopStart

any ideas on how to make this not close when the ping passes? I'm a little stumped it all works fine when the ping fails - loops and records as desired.

If you 'want' quotes in `set`, the opening quote is put *before* the variable name, like this: `set "folder1=C:\Users\Joseph\Desktop\Testing_1"`. So instead of `set file1="..."`, it should be `set "file1=%folder1%\25..."` — Poypoyan, Jul 15 '20 at 18:00
See [debugging a batch file](https://stackoverflow.com/a/42448601/3074564) and [Why is no string output with 'echo %var%' after using 'set var = text' on command line?](https://stackoverflow.com/a/26388460/3074564) The environment variable `file1` is defined with `""C:\Users\Joseph\Desktop\Testing_1"\25090_log_1.txt"`. Do you see what is wrong with the quotes? Run in command prompt window also `set /?` and read in output help the section about __arithmetic expressions__. It is better to use `set /a num+=1` instead of `set /a num=%num% + 1`. — Mofi, Jul 15 '20 at 18:20
So my outputs are functioning as desired, and the script functions without issues when the error is present. im more wondering why the CMD closes instead of continuously looping as desired when there is not error for it - EG why is the GoTo loop start and timeout only working if the error is present? — Joseph Dean, Jul 15 '20 at 18:24
Next I suggest to insert a label above `timeout %INTERVAL%` like `:ProcessWait` and use as first condition `if not errorlevel 1 goto ProcessWait`. Then the next four command lines can be without `if errorlevel 1`. See also [What are the ERRORLEVEL values set by internal cmd.exe commands?](https://stackoverflow.com/questions/34987885/) and [Which cmd.exe internal commands clear the ERRORLEVEL to 0 upon success?](https://stackoverflow.com/questions/34968009/) — Mofi, Jul 15 '20 at 18:24
Ahhhh.. i like that idea - the if not makes sense - i dont know why im not looking at this like a VBA script ... makes sense ill go test :) my only concern is if the addition of that label will break the existing portion... only a test will let me know for sure though. ill be back shortly thank you @mofi for the feedback and ideas — Joseph Dean, Jul 15 '20 at 18:26

Compo · Answer 1 · 2020-07-15T19:38:27.663

I'm not sure what your issue is, but looking at the script you've provided , I think it would be clearer like this:

@Echo Off
SetLocal EnableExtensions DisableDelayedExpansion

Rem setting variables/conditions for the loop
Rem note interval is number of seconds between loop run

Set "folder1=%USERPROFILE%\Desktop\Testing_1"
Set "IPAddress=8.8.8.8"
Set "Interval=60"
Set "num=0"

:LoopStart
"%__APPDIR__%PING.EXE" -n 4 %IPAddress% ^
 | "%__APPDIR__%find.exe" "TTL=" || (Set /A num +=1
    SetLocal EnableDelayedExpansion
    1> "%folder1%\25090_log_!num!.txt" (Echo %DATE%-%TIME%
        "%__APPDIR__%PING.EXE" -n 1 %IPAddress%)
    EndLocal)
"%__APPDIR__%timeout.exe" /T %Interval% /NoBreak
GoTo LoopStart

If you're not happy with using the conditional ||, you could still use If ErrorLevel:

@Echo Off
SetLocal EnableExtensions DisableDelayedExpansion

Rem setting variables/conditions for the loop
Rem note interval is number of seconds between loop run

Set "folder1=%USERPROFILE%\Desktop\Testing_1"
Set "IPAddress=8.8.8.8"
Set "Interval=60"
Set "num=0"

:LoopStart
"%__APPDIR__%PING.EXE" -n 4 %IPAddress% ^
 | "%__APPDIR__%find.exe" "TTL="
If ErrorLevel 1 (Set /A num +=1
    SetLocal EnableDelayedExpansion
    1> "%folder1%\25090_log_!num!.txt" (Echo %DATE%-%TIME%
        "%__APPDIR__%PING.EXE" -n 1 %IPAddress%)
    EndLocal)
"%__APPDIR__%timeout.exe" /T %Interval% /NoBreak
GoTo LoopStart

However, if it's delayed expansion you're daunted by, then this is more like I'd have expected from Mofi's advice.

@Echo Off
SetLocal EnableExtensions

Rem setting variables/conditions for the loop
Rem note interval is number of seconds between loop run

Set "folder1=%USERPROFILE%\Desktop\Testing_1"
Set "IPAddress=8.8.8.8"
Set "Interval=60"
Set "num=0"

:LoopStart
"%__APPDIR__%PING.EXE" -n 4 %IPAddress% ^
 | "%__APPDIR__%find.exe" "TTL="
If Not ErrorLevel 1 GoTo ProcessWait
Set /A num +=1
1> "%folder1%\25090_log_%num%.txt" (Echo %DATE%-%TIME%
    "%__APPDIR__%PING.EXE" -n 1 %IPAddress%)

:ProcessWait
"%__APPDIR__%timeout.exe" /T %Interval% /NoBreak
GoTo LoopStart

ummm yeah much much clearer - i was having an issue with the CMD closing after the first run if the error wasnt present this was cleared up with Mofi's suggestion though which is testing as desired... unrelated ... you're better than i am at this :) — Joseph Dean, Jul 15 '20 at 18:42
Is it cleared up using the code above @JosephDean? If so, you should either accept it as an answer, or upvote it and post your own code, _(as not cleared up with Mofi's commented help)_, and accept your own solution as accepted. — Compo, Jul 15 '20 at 18:51
im letting the suggestion he provided run on my test machine to see if it fails - also i did upvote your code earlier because it looks very nice. unfortunately because im a brand new account per the site it doesnt change the public view - i did up vote it though :( — Joseph Dean, Jul 15 '20 at 19:03
just to clarify I had a typo in my last comment which should have read: _(as now cleared up with Mofi's commented help)_. If you decide to post and go with your own code, and still do not have the reputation to upvote my answer, then see if the 'say thanks for this answer' button works for it instead. — Compo, Jul 15 '20 at 19:03
yes i was allowing my test to run to see if it would break or fail, which it hasn't - also i cannot accept my own response as the answer for two days per the sites settings... — Joseph Dean, Jul 15 '20 at 19:08

score 0 · Answer 2 · answered Jul 15 '20 at 19:06

0

@echo off

:: setting variables/conditions for the loop
:: note interval is number of seconds between loop run

    set folder1="C:\Users\Joseph\Desktop\Testing_1"
    set num=0
    Set Interval=5
    set IPAddress=8.8.8.8
:LoopStart
    ping -n 4 %IPADDRESS% | find "TTL=" > nul
        if not errorlevel 1 goto ProcessWait    
        if errorlevel 1 set /a num=%num% + 1
        if errorlevel 1 set file1="%folder1%\25090_log_%num%.txt"
        if errorlevel 1 echo %date%-%time% >> %file1%
        if errorlevel 1 ping -n 1 %ipaddress% >>%file1%
:ProcessWait
    timeout %INTERVAL%
Goto LoopStart

corrected and tested thank you @mofi for the commented suggestions worked perfectly

answered Jul 15 '20 at 19:06

Joseph Dean

11
1

Regardless of your decision on which answer to choose, I would advise that you do not use your code like that. All of those `if errorlevel` commands are not very good, and do not fully follow the advice @Mofi provided. You'd have been better off with a single `if errorlevel` with parenthesized commands, or none of them whatsover. You are also opening a file writing to it, closing it, opening it again, writing to it and closing it again, which is wasting time and resources unnecessarily. Please see my answer(s), _where I've included a couple of additional examples, to show you_. – Compo Jul 15 '20 at 19:33
All `if errorlevel 1` are definitely not needed according to my suggestion. – Mofi Jul 15 '20 at 19:34
i appreciate your examples, the code i posted works for that i am trying to get it to do. I will continue to read up and learn to make code you "expect to see" – Joseph Dean Jul 15 '20 at 20:26

Io-oI · Answer 3 · 2020-07-17T09:56:34.797

Porting an option involving less code and using the || operator would be:

@echo off

setLocal EnableDelayedExpansion
set "_IP=8.8.8.8" & set /a "_Int=5, _Num=0"
set "_Folder=%UserProfile%\Desktop\Testing_1"

:LoopStart
    %__APPDIR__%\ping.exe -n 4 !_IP!|find "TTL=" >nul && goto=:ProcessWait ||(
         set /a "_Num+=1" && call set "_File=!_Folder!\25090_!_Num!.txt"
         >>"!_File!" (echo;!date!-!time! & %__APPDIR__%\ping.exe -n 1 !_IP! ))
        
:ProcessWait
     %__APPDIR__%timeout.exe /nobreak !_Int! & goto=:LoopStart

I would recommend using return 0/return non 0 instead of errorlevel 1, either the execution was successful or not successful.

What is done by the && and || operator.

The actions that will take place after a successful or unsuccessful execution, are assumed by the operator, who deals with unquestionable reliability.

Return 0 or return non 0 is what is done by the operator with && and || ...

_{Note: I'm not saying/affirm that errorlevel is less or more reliable.}

@echo off

setLocal EnableDelayedExpansion
set "_IP=8.8.8.8" & set /a "_Int=5"
set "_Folder=%UserProfile%\Desktop\Testing_1"

:LoopStart
    %__APPDIR__%\ping.exe -n 4 !_IP!|find "TTL=" >nul && goto=:ProcessWait ||(
         set /a "_Num+=1+0" && call set "_File=!_Folder!\25090_!_Num!.txt"
         >>"!_File!" (echo;!date!-!time! & %__APPDIR__%\ping.exe -n 1 !_IP! ))
        
:ProcessWait
     %__APPDIR__%timeout.exe /nobreak !_Int! & goto=:LoopStart

If you need to declare _num before entering the loop, you can declare/set multiple numeric variables in one line:

 set /a "var1=x, var2=y, varN=n"

CMD closing instead of looping

3 Answers3

Note: I'm not saying/affirm that errorlevel is less or more reliable.

_{Note: I'm not saying/affirm that errorlevel is less or more reliable.}