On 09.04.2025 13:28, Alejandro Vallejo wrote:
> On Wed Apr 9, 2025 at 12:11 PM BST, Alejandro Vallejo wrote:
>> On Wed Apr 9, 2025 at 7:48 AM BST, Jan Beulich wrote:
>>> On 08.04.2025 18:07, Alejandro Vallejo wrote:
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -978,10 +978,30 @@ static unsigned int __init copy_bios_e820(struct
>>>> e820entry *map, unsigned int li
>>>> return n;
>>>> }
>>>>
>>>> -static struct domain *__init create_dom0(struct boot_info *bi)
>>>> +static size_t __init domain_cmdline_size(
>>>> + struct boot_info *bi, struct boot_domain *bd)
>>>
>>> const for both? And perhaps s/domain/dom0/ in the function name?
>
> (missed this one)
>
> Sure to the const pointers. But as the hyperlaunch effort progresses the
> point is to turn all this into a more generic domain builders rather
> than having dom0-specific stuff. Changing the name like that here to
> adjust it in a few patches down the line doesn't seem worth the effort.
Oh, if the function is going to fine further uses, that's likely okay.
("likely" because we've seen too many abandoned series, where we then
ended up with pieces that were never gaining their intended final
purpose.)
>>> While this tidies the local variable, what about bd->cmdline? As it stands
>>> this
>>> gives the impression that you're freeing a pointer here which may still be
>>> used
>>> through passing bd elsewhere.
>>
>> That ought to have been bd->cmdline indeed.
>>
>
> Actually, it can't be. It's a "const char *", so XFREE() chokes on it.
> I'll turn it into
>
> XFREE(cmdline);
> bd->cmdline = NULL;
>
> instead.
At which point it can be xfree() again, seeing that the function ends
right afterwards?
Jan
