We currently require full argument specification:

  virt-admin daemon-timeout --timeout X

Well, the '--timeout' feels a bit redundant. Turn the argument
into a positional so that the following works too:

  virt-admin daemon-timeout X

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 docs/manpages/virt-admin.rst | 2 +-
 tools/virt-admin.c           | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/docs/manpages/virt-admin.rst b/docs/manpages/virt-admin.rst
index 5108781636..54a6512ef7 100644
--- a/docs/manpages/virt-admin.rst
+++ b/docs/manpages/virt-admin.rst
@@ -320,7 +320,7 @@ daemon-timeout
 
 ::
 
-   daemon-timeout --timeout NUM
+   daemon-timeout [--timeout] NUM
 
 Sets the daemon timeout to the value of '--timeout' argument. Use ``--timeout 0``
 to disable auto-shutdown of the daemon.
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index 1805618035..3eb4f0f3fd 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1009,6 +1009,7 @@ static const vshCmdOptDef opts_daemon_timeout[] = {
     {.name = "timeout",
      .type = VSH_OT_INT,
      .required = true,
+     .positional = true,
      .help = N_("number of seconds the daemon will run without any active connection"),
     },
     {.name = NULL}
-- 
2.44.1

On Mon, May 27, 2024 at 11:18:50 +0200, Michal Privoznik wrote:
> We currently require full argument specification:
> 
>   virt-admin daemon-timeout --timeout X
> 
> Well, the '--timeout' feels a bit redundant. Turn the argument
> into a positional so that the following works too:
> 
>   virt-admin daemon-timeout X
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  docs/manpages/virt-admin.rst | 2 +-
>  tools/virt-admin.c           | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)

It was a bit of a deliberate decision, because any positional argument
can't be made optional and must be kept in order. Historically also the
parser did weird things for stuff that wasn't really considered
positional but was parsed so regardless.

In this case though ... I can't really see another option being added or
timeout becoming optional especially since '0' is used as way to disable
the timeout.

So your justification here makes sense, it's just that I really don't
like positional arguments.

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On 5/27/24 12:55, Peter Krempa wrote:
> On Mon, May 27, 2024 at 11:18:50 +0200, Michal Privoznik wrote:
>> We currently require full argument specification:
>>
>>   virt-admin daemon-timeout --timeout X
>>
>> Well, the '--timeout' feels a bit redundant. Turn the argument
>> into a positional so that the following works too:
>>
>>   virt-admin daemon-timeout X
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  docs/manpages/virt-admin.rst | 2 +-
>>  tools/virt-admin.c           | 1 +
>>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> It was a bit of a deliberate decision, because any positional argument
> can't be made optional and must be kept in order. Historically also the
> parser did weird things for stuff that wasn't really considered
> positional but was parsed so regardless.
> 
> In this case though ... I can't really see another option being added or
> timeout becoming optional especially since '0' is used as way to disable
> the timeout.

This was exactly my internal reasoning when writing this patch because I
too dislike positional arguments.

> 
> So your justification here makes sense, it's just that I really don't
> like positional arguments.

Since our docs are fixed by previous patch, this one is just a
'shortcut' and strictly speaking it's not needed. I can drop it, I don't
mind.

> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> 

Michal