Re: [PATCH v2] tools/xl: fix channel configuration setting

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2846 bytes --]

On 28.02.25 18:34, Anthony PERARD wrote:
> On Thu, Feb 27, 2025 at 04:10:25PM +0100, Jürgen Groß wrote:
>> On 26.02.25 16:55, Anthony PERARD wrote:
>>> On Tue, Feb 25, 2025 at 08:30:33AM +0100, Juergen Gross wrote:
>>>> Channels work differently than other device types: their devid should
>>>> be -1 initially in order to distinguish them from the primary console
>>>> which has the devid of 0.
>>>>
>>>> So when parsing the channel configuration, use
>>>> ARRAY_EXTEND_INIT_NODEVID() in order to avoid overwriting the devid
>>>> set by libxl_device_channel_init().
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> This might want a:
>>>       Fixes: 3a6679634766 ("libxl: set channel devid when not provided by application")
>>>
>>> Before that, the devid set by `xl` was probably ignored. I think before,
>>> the console devid would be taken from libxl__init_console_from_channel()
>>> parametters, so the first devnum would be `0+1`, so never 0.
>>>
>>> Do you agree?
>>
>> I'm not sure I do. The use of ARRAY_EXTEND_INIT() in xl_parse.c predates
>> commit 3a6679634766, so there is certainly more than one potential Fixes:
>> candidate.
> 
> Of course, this was my first candidate as well, but that's part of the
> original introduction of channel to the source code, so it didn't really
> make sense to that the feature was commited with devid=0, surely it
> would already been an issue at the time. This is why I start looking
> in libxl on how devid was used.
> 
>> So at least for the xl case commit 3a6679634766 isn't relevant, and my patch
>> is fixing the xl case only.
> 
> Last time I check, `xl` is using libxl, so any change to libxl can
> impact xl.

Of course, but with xl devid was never -1, so 3a6679634766 shouldn't make
a difference.

> Before 3a6679634766, channel#0 would be connected to console#1 because
> libxl__init_console_from_channel() was called with dev_num=`channel#+1`,
> and $dev_num was used to set console.devid.
> 
> After 3a6679634766, we have console.devid=channel.devid=dev_num if
> channel.devid is -1, or console.devid=channel.devid.
> 
> Without 3a6679634766, having more than one channel with devid=-1 would
> result in failing to start due to having more than one channel with the
> same devid.

Do you have any prove that more than one channel ever worked?

> 
> So the only good candidate is:
> Fixes: 3a6679634766 ("libxl: set channel devid when not provided by application")

In the end I don't feel strong with this. If you really want, I can add
this Fixes tag.

> Also, I'm not sure channels are going to work properly if there's more
> than one console, lots of code seems to assume
> console.devid=channel.devid+1, and there's probably other issues.

I believe channels are not tested a lot.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]