The timeout argument for guest-agent-timeout is optional but it did not
have proper default value specified. Also update the virsh man page
accordingly.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 docs/manpages/virsh.rst | 7 ++++---
 tools/virsh-domain.c    | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index 92de0b2192..6e48ae7973 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -2631,15 +2631,16 @@ guest
 
 .. code-block::
 
-   guest-agent-timeout domain --timeout value
+   guest-agent-timeout domain [--timeout value]
 
 Set how long to wait for a response from guest agent commands. By default,
 agent commands block forever waiting for a response. ``value`` must be a
 positive value (wait for given amount of seconds) or one of the following
 values:
 
-* -2 - block forever waiting for a result,
-* -1 - reset timeout to the default value,
+* -2 - block forever waiting for a result (used when --timeout is omitted),
+* -1 - reset timeout to the default value (currently defined as 5 seconds in
+  libvirt daemon),
 * 0 - do not wait at all,
 
 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 286cf79671..1f3a549d9a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -14207,7 +14207,7 @@ static bool
 cmdGuestAgentTimeout(vshControl *ctl, const vshCmd *cmd)
 {
     virDomainPtr dom = NULL;
-    int timeout;
+    int timeout = VIR_DOMAIN_AGENT_RESPONSE_TIMEOUT_BLOCK;
     const unsigned int flags = 0;
     bool ret = false;
 
-- 
2.25.0

On 8/21/20 2:34 PM, Tomáš Golembiovský wrote:
> The timeout argument for guest-agent-timeout is optional but it did not
> have proper default value specified. Also update the virsh man page
> accordingly.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>   docs/manpages/virsh.rst | 7 ++++---
>   tools/virsh-domain.c    | 2 +-
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

On Fri, Aug 21, 2020 at 02:34:51PM +0200, Tomáš Golembiovský wrote:
> The timeout argument for guest-agent-timeout is optional but it did not
> have proper default value specified. Also update the virsh man page
> accordingly.
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
>  docs/manpages/virsh.rst | 7 ++++---
>  tools/virsh-domain.c    | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index 92de0b2192..6e48ae7973 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -2631,15 +2631,16 @@ guest
>
>  .. code-block::
>
> -   guest-agent-timeout domain --timeout value
> +   guest-agent-timeout domain [--timeout value]
>
>  Set how long to wait for a response from guest agent commands. By default,
>  agent commands block forever waiting for a response. ``value`` must be a
>  positive value (wait for given amount of seconds) or one of the following
>  values:
>
> -* -2 - block forever waiting for a result,
> -* -1 - reset timeout to the default value,
> +* -2 - block forever waiting for a result (used when --timeout is omitted),

--timeout could have been tagged with '*' for underscoring.

> +* -1 - reset timeout to the default value (currently defined as 5 seconds in

I agree that when "default" is mentioned we should document what the default is
otherwise it kinda loses the point to mention it. On the other hand, if we
change it in the future, users of the old libraries relying on the documented
value may be surprised it's no longer 5 seconds, so I'm a bit hesitant to
mention it in the manpage. Honestly, this is the kind of default value that IMO
doesn't make much sense to expose in the first place, either you're fine with
blocking forever or set a timeout yourself. If anything, we could mention that
one should set timeout explicitly if blocking is not acceptable.

Erik

On 8/24/20 12:10 PM, Erik Skultety wrote:
> On Fri, Aug 21, 2020 at 02:34:51PM +0200, Tomáš Golembiovský wrote:
>> The timeout argument for guest-agent-timeout is optional but it did not
>> have proper default value specified. Also update the virsh man page
>> accordingly.
>>
>> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
>> ---
>>   docs/manpages/virsh.rst | 7 ++++---
>>   tools/virsh-domain.c    | 2 +-
>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
>> index 92de0b2192..6e48ae7973 100644
>> --- a/docs/manpages/virsh.rst
>> +++ b/docs/manpages/virsh.rst
>> @@ -2631,15 +2631,16 @@ guest
>>
>>   .. code-block::
>>
>> -   guest-agent-timeout domain --timeout value
>> +   guest-agent-timeout domain [--timeout value]
>>
>>   Set how long to wait for a response from guest agent commands. By default,
>>   agent commands block forever waiting for a response. ``value`` must be a
>>   positive value (wait for given amount of seconds) or one of the following
>>   values:
>>
>> -* -2 - block forever waiting for a result,
>> -* -1 - reset timeout to the default value,
>> +* -2 - block forever waiting for a result (used when --timeout is omitted),
> 
> --timeout could have been tagged with '*' for underscoring.
> 
>> +* -1 - reset timeout to the default value (currently defined as 5 seconds in
> 
> I agree that when "default" is mentioned we should document what the default is
> otherwise it kinda loses the point to mention it. On the other hand, if we
> change it in the future, users of the old libraries relying on the documented
> value may be surprised it's no longer 5 seconds, so I'm a bit hesitant to
> mention it in the manpage. Honestly, this is the kind of default value that IMO
> doesn't make much sense to expose in the first place, either you're fine with
> blocking forever or set a timeout yourself. If anything, we could mention that
> one should set timeout explicitly if blocking is not acceptable.
> 

Changing any default, even a documented one will always result in pain. 
If an app relies on a specific timeout value then the best would be to 
simply pass it. If the app trust us to chose a sensible default then it 
can omit the argument.

We document a lot of defaults, and a lot of them use "future proof" 
wording like "hypervisor default" so that we have our backs covered.

I think the documentation is correct (the default is 5 seconds, currently).

Michal