On 14.04.2025 19:06, Alejandro Vallejo wrote:
> On Thu Apr 10, 2025 at 12:34 PM BST, Jan Beulich wrote:
>> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>>> --- a/xen/arch/x86/domain-builder/fdt.c
>>> +++ b/xen/arch/x86/domain-builder/fdt.c
>>> @@ -195,6 +195,35 @@ static int __init process_domain_node(
>>> !((char *)__va(bd->kernel->cmdline_pa))[0] )
>>> bd->kernel->fdt_cmdline = fdt_get_prop_offset(
>>> fdt, node, "bootargs", &bd->kernel->cmdline_pa);
>>> +
>>> + continue;
>>
>> With this ...
>>
>>> + }
>>> + else if ( fdt_node_check_compatible(fdt, node,
>>
>> ... no need for "else" here?
>
> Sure
>
>>
>>> + "multiboot,ramdisk") == 0 )
>>> + {
>>> + int idx;
>>> +
>>> + if ( bd->module )
>>> + {
>>> + printk(XENLOG_ERR "Duplicate ramdisk module for domain
>>> %s)\n",
For context below, note this message.
>>> + name);
>>> + continue;
>>> + }
>>> +
>>> + idx = fdt_read_multiboot_module(fdt, node, address_cells,
>>> + size_cells,bi);
>>> + if ( idx < 0 )
>>> + {
>>> + printk(" failed processing ramdisk module for domain
>>> %s\n",
>>> + name);
>>> + return -EINVAL;
>>> + }
>>
>> Along the lines of what Denis has said - please be consistent about log
>> messages: XENLOG_* or not, preferably no capital at the start, initial
>> blank padding. May apply elsewhere in the series as well.
>
> I don't mind dropping that and making everything flat (uppercase + no
> padding), but there is some consistency. Albeit, it is true the
> rationale is somewhat obscure.
Obscure or not - it might be fine if stated that way sufficiently
prominently, such that future additions here then will adhere to that
model.
> ATM the consistency is: "padding spaces + lowercase" when giving extra
> information on hyperlaunch. It ends up creating a hyperlaunch block in
> `dmesg` with a "Hyperlaunch detected" line on top so it's easier to
> know what lines are hyperlaunch related and which ones aren't.
>
> Do you have a preference for a specific reporting style?
XENLOG_* or not want to be consistent, no matter what. Generally I think
that log messages shouldn't start with capitals, unless it's e.g. an
acronym. The padding to help grouping would be fine with me if, as said,
properly spelled as the scheme to use.
Jan
