Re: [PATCH v8 1/3] monitor/hmp: add support for flag argument with value

[Markus Armbruster]

Fabian Ebner <f.ebner@proxmox.com> writes:

> Am 09.02.22 um 15:12 schrieb Markus Armbruster:
>> Fabian Ebner <f.ebner@proxmox.com> writes:
>
> ----8<----
>
>>> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
>>> index 3da3f86c6a..a4cb307c8a 100644
>>> --- a/monitor/monitor-internal.h
>>> +++ b/monitor/monitor-internal.h
>>> @@ -63,7 +63,8 @@
>>>   * '.'          other form of optional type (for 'i' and 'l')
>>>   * 'b'          boolean
>>>   *              user mode accepts "on" or "off"
>>> - * '-'          optional parameter (eg. '-f')
>>> + * '-'          optional parameter (eg. '-f'); if followed by a 'V', it
>>> + *              specifies an optional string param (e.g. '-fV' allows '-f 
>>> foo')
>>>   *
>>>   */
>> 
>> For what it's worth, getopt() uses ':' after the option character for
>> "takes an argument".
>> 
>
> Doing that leads to e.g.
>     .args_type  = "protocol:s,password:s,display:-d:,connected:s?",
> so there's two different kinds of colons now.

Point.

>                                               It's not a problem
> functionality-wise AFAICT, but it might not be ideal. Should I still go
> for it?
>
> Also, wouldn't future non-string flag parameters need their own letter
> too? What about re-using 's' here instead?

Another good point.

Dave, what do you think?

>> Happy to make that tweak in my tree.  But see my review of PATCH 3
>> first.