problem-specific error codes?

[r2evans]

For discussion: what are your thoughts on returning different error codes? For instance, I can see utility in differentiating between hard and transient errors. I could loop on sending a message until success. If the send is failing because of a config problem, then it will never succeed (and loop needlessly).

I don't mind working on a PR for it, just wanted to know your (@Red5d's) opinion before going down this road. If generally "yes", could you help enumerate what error codes would make sense?

• 1-99: return codes passed through from curl (e.g., 6 is "couldn't resolve host", 7 is "failed to connect to host", both possibly transient errors)
• 200: PB authentication error
• 201: PB device error (not found or already exists?)
• 202: PB object-not-found error
• 203: PB other error
• 210: dependency error (hard fail)
• 211: configuration error
• 212: URL or email format error
• 213: file error (perms or not found)

Since the current method is to always return "1" if there's an error, I don't see backward compatibility being an issue here. Since man curl only lists up to 90 at the moment, it should be easy to start at 100; I chose 200 only to preserve plenty of room for curl's error-code expansion.

[fbartels]

Thanks for your pull request, this has now been merged.

[fbartels]

On a related note. in #82 you gave the following snippet:

MSG="hello world, my external IP changed to ${WANIP}"
./pushbullet push mydev note "${MSG}"
retcode=$?
while [ "$retcode" = "5" -o "$retcode" = "6" -o "$retcode" = "7" ]; do
    sleep 300
    ./pushbullet push mydev note "${MSG}"
    retcode=$?
done

What would you think if such an error handling would be directly included in pushbullet-bash? e.g. if curl gives a network error retry for a given amount of times before failing. this given amount could be configurable as well imho.

[r2evans]

I had considered it, but didn't want to get too far ahead of myself with regards to suggested-changes.

Perhaps along the lines of:

 printUsage() {
-echo "Usage: pushbullet [-q|--quiet|-d|--debug] <action> <device> <type> <data>
+echo "Usage: pushbullet [-q|--quiet|-d|--debug|-R [-E...|-D...|-T...] ] <action> <device> <type> <data>
 
-Options:
+General Options:
 -q, --quiet - only print error messages.
 -d, --debug - print debug output
+
+Retry Options:
+-R, --retry - retry failed connection attempts, if failure due to curl
+              transient network errors
+-E ERRS, --retry-errors ERRS
+            - comma-delimited error codes that will allow --retry
+              (default "5,6,7")
+-T INT, --retry-times INT
+            - number of times to retry after the first failed attempt;
+              0 means only once, -1 means non-stop (default 2, for a total
+              of 3 connection attempts)
+-D INT, --retry-delay INT
+            - pause (in seconds) between retry attempts, 0 means instantly
+              (default 60); Note: delays under 30 seconds will abort unless
+              '--force-short-delay' is also provided, in order to prevent
+              inadvertent denial-of-service on the foreign API
+--force-short-delay
+            - a delay time of less than 30 seconds can be inconsiderate
+              to the foreign API, this option allows the user to set delay
+              times below that threshold

 Actions:
 ...

It can be argued that --retry is not strictly necessary: if the default is set to --retry-times 0, then --retry-delay and --retry-errors are completely moot. Since this is reaching out to a foreign API that may or may not have rate-limiting or count-limiting, however, the extra -R is a way to discourage casual and inadvertent DOS. This way, the if -R alone is set, then sane defaults for the others are used (60 secs, 2 retries for a total of 3 attempts).

I waffle on using --retry-times (which count after the first failed attempt) versus --tries or --attempts (which would include the first failed attempt).

[fbartels]

hm.. adding that many optional parameters then also would call for switching to getops. The approach I had in mind would be to simply define some new default variables that can be overriden in .config/pushbullet (just like your curl options). By default the script could fail on the obvious connection errors (5,6,7 just like above) and retry every 60 seconds for at least five times.

[r2evans]

No need for getopts (which would subsequently break the POSIX-compliant shell PR just merged).
getopts works in /bin/sh as well (my bad), though it does not support long-options. If you want short/long alternatives, then the below code would be useful, otherwise getopts would work.

With the assumption that variable arguments are placed before "action", it's easy enough to use something like (untested):

PARSEARGS=1
while [ -n "${PARSEARGS}" && "$#" -gt "0" ]; do
    case "$1" in
        -R|--retry)
            DORETRY=1
            ;;
        -E|--retry-errors)
            shift
            ERRCODES=$1
            ;;
        -T|--retry-times)
            shift
            RETRYTIMES=$1
            ;;
        -D|--retry-delay)
            shift
            RETRYDELAY=$1
            ;;
        --force-short-delay)
            RETRYDELAYSHORT=1
            ;;
        *)
            unset PARSEARGS
            ;;
    esac
    if [ -n "${PARSEARGS}" ]; then shift ; fi
done

In general I like the thought of storing defaults in ~/.config/pushbullet, though we need to make sure that the arguments allow "easily disabling the default"; for example, if the default is (say) -T 2, then using -T 0 should ensure no retries are attempted.

[r2evans]

Let's continue this conversation in #83.