Add command line timeout parameter
Closed this issue · 3 comments
It may be good to allow the user to specify the timeout on the command line. Additionally it may be good for us to change the default timeout to #define DBUS_TIMEOUT_INFINITE ((int) 0x7fffffff).
Timeout ref. https://dbus.freedesktop.org/doc/api/html/group__DBusPendingCall.html#ga21384c9c5b0da54f7d0a92012522f213
This is both trickier and easier than I had anticipated.
It looks like in _actions/_data.py as the module is loaded, it should be possible to read the designated environment variable if present, and use it rather than the defined constant, DBUS_TIMEOUT_SECONDS in the calls to the various code generation methods.
dbus-python seems to expect this value to be in seconds, and as a floating point number, so we will have to stick w/ that.
Note that the special value meaning infinite timeout is an int, the maximum possible positive value in a 32-bit int. The current hypothesis seems to be that this represents milliseconds, not seconds, based on reading dbus-python code.
To use the special value standing for infinite timeout, we would have to choose a float that would somehow be converted to the precise int designated. There's no way around this, so we are stuck reasoning about floating point numbers, and seeing if it is even possible to do this. We might also consider filing a bug about this infelicitous situation against dbus-python. Our problem: does there exists a floating point number, f, such that (int)(f * 1000.0) is equal to INT_MAX for 32 bit ints. This is fairly easy to check numerically, I would think.
Yes. The floating point number is exactly 2147483.647. But we will have to verify this with more care than I've been able to give it here, and we may not want to expose it to users. We might just want to go w/ stating that the largest possible timeout is 2147483 seconds, which is slightly less than a month, but not a lot.
In standup it seemed much more reasonable to have the environment variable be in milliseconds than to allow fractional seconds, so we should go with that.
If the timeout value is too large, stratis gets this:
[~/stratis2/stratis-cli (master)]$ PYTHONPATH=./src ./bin/stratis daemon version
stratis encountered an unexpected error during execution. Please report the error and include in your report the stack trace shown below.
Traceback (most recent call last):
File "/home/mulhern/stratis2/stratis-cli/src/stratis_cli/_main.py", line 42, in the_func
result.func(result)
File "/home/mulhern/stratis2/stratis-cli/src/stratis_cli/_actions/_stratis.py", line 45, in list_stratisd_version
print("%s" % Manager.Properties.Version.Get(get_object(TOP_OBJECT)))
File "/usr/local/lib/python3.7/site-packages/dbus_python_client_gen-0.7-py3.7.egg/dbus_python_client_gen/_invokers.py", line 68, in dbus_func
File "/usr/lib64/python3.7/site-packages/dbus/proxies.py", line 145, in __call__
**keywords)
File "/usr/lib64/python3.7/site-packages/dbus/connection.py", line 651, in call_blocking
message, timeout)
ValueError: Timeout too long
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "./bin/stratis", line 35, in <module>
main()
File "./bin/stratis", line 31, in main
return run()(sys.argv[1:])
File "/home/mulhern/stratis2/stratis-cli/src/stratis_cli/_main.py", line 56, in the_func
handle_error(err)
File "/home/mulhern/stratis2/stratis-cli/src/stratis_cli/_error_reporting.py", line 121, in handle_error
raise err
File "/home/mulhern/stratis2/stratis-cli/src/stratis_cli/_main.py", line 51, in the_func
raise StratisCliActionError(command_line_args, result) from err
stratis_cli._errors.StratisCliActionError: Action selected by command-line arguments ['daemon', 'version'] which were parsed to Namespace(func=<function StratisActions.list_stratisd_version at 0x7fdef78041e0>, propagate=False) failed
dbus-python is raising a simple ValueError, which is unfortunate. In the best case, only the interpreter would raise ValueErrors, and libraries would define their own errors for these kinds of things. Under these circumstances, I think it would make sense for the CLI to verify that the timeout value is within the valid range (rather than set itself up to catch that ValueError, which would be tedious).