Re: [PATCH] xen/argo: Command line handling improvements

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2025 20:45, Daniel P. Smith wrote:
> On 5/20/25 10:10, Andrew Cooper wrote:
>> Treat "argo" on the command line as a positive boolean, rather than requiring
>> the user to pass "argo=1/on/enable/true".
>>
>> Move both opt_argo* variables into __ro_after_init.  They're set during
>> command line parsing and never modified thereafter.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Christopher Clark <christopher.w.clark@xxxxxxxxx>
>> CC: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> CC: Denis Mukhin <dmkhn@xxxxxxxxx>
>>
>> Found while
>> ---
>>   xen/common/argo.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/common/argo.c b/xen/common/argo.c
>> index cbe8911a4364..027b37b18a6c 100644
>> --- a/xen/common/argo.c
>> +++ b/xen/common/argo.c
>> @@ -79,8 +79,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_argo_unregister_ring_t);
>>   DEFINE_COMPAT_HANDLE(compat_argo_iov_t);
>>   #endif
>>   
>> -static bool __read_mostly opt_argo;
>> -static bool __read_mostly opt_argo_mac_permissive;
>> +static bool __ro_after_init opt_argo;
>> +static bool __ro_after_init opt_argo_mac_permissive;
>>   
>>   static int __init cf_check parse_argo(const char *s)
>>   {
>> @@ -92,7 +92,10 @@ static int __init cf_check parse_argo(const char *s)
>>           if ( !ss )
>>               ss = strchr(s, '\0');
>>   
>> -        if ( (val = parse_bool(s, ss)) >= 0 )
>> +        /* Intepret "argo" as a positive boolean. */
>> +        if ( *s == '\0' )
>> +            opt_argo = true;
>> +        else if ( (val = parse_bool(s, ss)) >= 0 )
>>               opt_argo = val;
>>           else if ( (val = parse_boolean("mac-permissive", s, ss)) >= 0 )
>>               opt_argo_mac_permissive = val;
>>
>> base-commit: 293abb9e0c5e8df96cc5dfe457c62dfcb7542b19
> 
> While it is logical, this does technically change the behavior of the 
> command line flag. Should there be an update to xen-command-line.pandoc 
> to clarify that the list is optional?

I'd view it the other way around: If you look at the neighboring doc
entries, we uniformly say

> `= <boolean>`

when the options can appear all by themselves. This would therefore be
the expected behavior for "argo" alone, too.

Jan