Re: [PATCH v6 03/12] x86/hyperlaunch: initial support for hyperlaunch device tree

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.05.2025 19:24, Alejandro Vallejo wrote:
> On Wed May 21, 2025 at 5:00 PM CEST, Jan Beulich wrote:
>> On 29.04.2025 14:36, Alejandro Vallejo wrote:
>>> --- a/xen/common/domain-builder/fdt.c
>>> +++ b/xen/common/domain-builder/fdt.c
>>> @@ -13,6 +13,36 @@
>>>  
>>>  #include "fdt.h"
>>>  
>>> +static int __init find_hyperlaunch_node(const void *fdt)
>>> +{
>>> +    int hv_node = fdt_path_offset(fdt, "/chosen/hypervisor");
>>> +
>>> +    if ( hv_node >= 0 )
>>> +    {
>>> +        /* Anything other than zero indicates no match */
>>> +        if ( fdt_node_check_compatible(fdt, hv_node, "hypervisor,xen") )
>>> +            return -ENODATA;
>>> +
>>> +        return hv_node;
>>> +    }
>>> +    else
>>
>> Please can such unnecessary (and potentially misleading) "else" be omitted?
> 
> Not sure how it could be misleading,

For context, just briefly looking at such a construct may leave one with
the (wrong) impression that both branches of the conditional can fall through
to subsequent code. This may be less of an issue as long as both sides are
reasonably short, but then it is imo better to avoid the pattern altogether.

Jan

> but...
> 
>> As ...
>>
>>> +    {
>>> +        /* Look for dom0less config */
>>> +        int node, chosen_node = fdt_path_offset(fdt, "/chosen");
>>
>> ... these will need to move to function scope then, one of the two may want
>> folding with "hv_node" above.
> 
> ... there is indeed a more compact form the function could take. Noted.
> 
> Cheers,
> Alejandro